jiantw83
d472f81cd2
現行紀錄只記「被叫用」，沒有成敗也沒有結束碼。跑完整輪的技能與開場就 中止的技能，在紀錄裡長得一模一樣。 start 由技能用量 hook 順手發，不必改技能文件。end 只能由技能自己在收尾 步驟寫——hook 接在技能工具呼叫上，而實際工作發生在之後的模型輪次，它在 原理上看不到成敗。有 start 沒有配對的 end，就是那一輪中止了。 status 五選一，每支技能各自寫明什麼情況選哪一個。找不到回報腳本就安靜 跳過，回報失敗一律不改變技能自己的結論。
81 lines
11 KiB
Markdown
81 lines
11 KiB
Markdown
---
|
|
name: code-review
|
|
description: Review changed code against the Refactoring smell catalog in six groups (bloaters, obscurity, couplers, dispensables, comment contract, shallow modules). Run when a file change is complete or an implementation is complete, e.g. from jsc-sdlc implement. Each group runs as a sub agent over the git diff; findings are reported with file:line, severity, and refactoring, and the caller decides whether to fix. Not a replacement for the CLI's built-in security or bug review.
|
|
---
|
|
|
|
# code-review
|
|
|
|
Review changed code against `references/smells.md` (from the book *Refactoring*). The reference is written in Traditional Chinese; read it as-is.
|
|
|
|
## When to run
|
|
|
|
1. **When a file change is complete**: one file or one related group of files is done.
|
|
2. **When an implementation is complete**: all todos of a work package are done. This is the call site in `jsc-sdlc:implement` — the end of a work package, after its last todo and before it is committed and turned into a PR.
|
|
|
|
## Division of labor
|
|
|
|
- This skill covers the *Refactoring* smells, the comment contract (group 5), and shallow modules (group 6).
|
|
- Security, logic bugs, and test coverage belong to the CLI's built-in review (e.g. claude's `/security-review`); do not duplicate them.
|
|
- Swagger document auditing belongs to `jsc-review:api-doc`: response type declarations per HTTP status code, Swagger parameter descriptions, and Swagger examples down the nested data models. Group 5 here covers the source comment contract only; the two never report the same gap twice.
|
|
|
|
## Steps
|
|
|
|
1. Snapshot the review scope once, to a path that belongs to this run. In one command, expand `${TMPDIR:-/tmp}/jsc-code-review.$$.diff` into a variable, redirect `git diff` (uncommitted changes) or `git diff {base}...HEAD` (against the base branch when an implementation is complete) into it, and print the expanded path. The `$$` is what keeps two runs on one machine apart; a fixed file name lets a second run overwrite the first one's snapshot mid-review, and the six groups in step 2 would then judge bytes that were never their own diff. Read git's exit code: `0` the snapshot is written; any non-zero code means git refused (not a repository, unknown base revision, unreadable object) — report git's stderr verbatim and stop, spawn no sub agent. An empty snapshot file → delete it, report the literal 「無發現」 and stop; spawn no sub agent. The six groups in step 2 all read this one file, so git computes the diff once and every group judges the same bytes. Completion condition: the expanded snapshot path is reported as a literal path — never as the unexpanded `$$` pattern — together with a non-empty changed-file list, or the run already ended with 「無發現」 or with git's error.
|
|
2. Review in six groups, and every group **MUST run as a sub agent**; the six groups may run in parallel:
|
|
|
|
| Group | Scope |
|
|
| --- | --- |
|
|
| 1 Bloaters | smells.md group 1 |
|
|
| 2 Obscurity | smells.md group 2. For 2.5 comment scope leaks, report only what a pattern cannot decide: project code names, customer names, and context-dependent review traces. `references/comment-scope.md` is the single source of the banned and allowed lists — read it, never restate it here. Pattern-detectable items belong to `jsc-hooks/hooks/comment-scope.sh` and stay outside this group (see Notes for what that leaves uncovered) |
|
|
| 3 Couplers | smells.md group 3 |
|
|
| 4 Dispensables & Others | smells.md group 4 |
|
|
| 5 Comment contract | smells.md group 5, 5.1 to 5.8 — including 5.6 navigation links to the functions a method calls, 5.7 XML comment tag layout (opening and closing tag each on its own line), and 5.8 code-element markup by language convention (`<paramref>`, `<see cref>`, `<c>` in XML; backticks in JSDoc and docstrings) |
|
|
| 6 Shallow Module | smells.md group 6 |
|
|
|
|
Instructions for each sub agent: read only, change nothing; read the diff snapshot at the exact path step 1 reported, that one file only, and never run `git diff` again; check every changed line and its enclosing function or class against the group's definitions and detection signals in smells.md.
|
|
|
|
Each sub agent returns its findings as TSV rows — one finding per row, six fields separated by a single tab, in this order:
|
|
|
|
| Field | Content |
|
|
| --- | --- |
|
|
| 1 file | path relative to the repository root |
|
|
| 2 line | line number, a positive integer |
|
|
| 3 severity | `高`, `中`, or `低` on the smells.md scale |
|
|
| 4 group | this group's number, `1` to `6` |
|
|
| 5 item | the smell name |
|
|
| 6 detail | one sentence of evidence plus the suggested refactoring |
|
|
|
|
Fields 5 and 6 are written in Traditional Chinese. Free-form prose is not accepted: `tools/merge-findings.sh` deduplicates on fields 1 and 2, so any row off this format is rejected and its finding never reaches the report. A group with nothing to report returns the single line 「無發現」 instead of rows.
|
|
|
|
Completion condition: all six groups have returned, each with TSV rows or with 「無發現」.
|
|
3. Merge with `tools/merge-findings.sh`. Concatenate the six groups' TSV rows in group order and pipe them into the script on stdin. Start this step only once all six groups have returned; a group still running means the merge waits. Read its exit code: `0` take its stdout as the merged list — deduplicated by location and sorted by severity, 高 first and 低 last; `1` no group returned a row, so report the literal 「無發現」 and stop; `2` environment or input error, so report the script's stderr verbatim and stop, and never hand-merge as a substitute; `3` a group returned a malformed row, and the stderr names which row, so ask that one group's sub agent to re-emit in the format above and run the merge again — after a second `3`, report the malformed rows verbatim and stop. Completion condition: the merged list is on hand with every location appearing exactly once, or the run ended with 「無發現」 or with the reported error.
|
|
4. Report the finding list in Traditional Chinese, one line per merged row. **This skill never modifies code.** `jsc-hooks/hooks/write-guard.sh` in `review` mode enforces that in code: while this skill runs, it blocks write tools. Which tools those are is declared in that script's own header; never restate the list here, because a stale copy of it reads as coverage the guard does not have. Only claude has a `PreToolUse` hook, so codex, copilot, antigravity, and kiro never reach that guard — on those four this rule and the sub agent instructions are the only constraint. The caller decides what to fix (inside the implementation flow, 高 and 中 are normally mandatory, 低 is judgment).
|
|
|
|
Then close the run down in two moves. Delete the step 1 snapshot file; nothing else ever reads it, and left behind it accumulates one stale diff per review. Release the review lock by running `jsc-hooks/hooks/write-guard.sh release`, which clears `$JSC_HOME/sessions/{sid}.lastskill`. That file is how the guard recognizes the running skill, and no event tells the guard a skill ended: leave it in place and the caller's first fix — the fix this very report asked for — is blocked by the audit that just finished. State the fallback in the report either way, because an older `jsc-hooks` treats `release` as an unknown mode and exits `0` without clearing anything: the lock then lifts by itself once the file is older than `JSC_WRITE_GUARD_TTL` (900 seconds by default), and `JSC_WRITE_GUARD=off` opens it immediately.
|
|
|
|
Completion condition: the report is handed to the caller, the snapshot is deleted, the release command has been run and the wait plus the `JSC_WRITE_GUARD=off` escape hatch are stated, and the fix decision is left to them.
|
|
5. Record how this run ended, as the very last thing this skill does — after the snapshot deletion and the release, so a cleanup that did not complete is still visible to it:
|
|
|
|
`jsc-hooks/tools/report-status.sh skill-end jsc-review:code-review {status} {exit} "{detail}"`
|
|
|
|
Resolve that path the way step 4 already resolves `jsc-hooks/hooks/write-guard.sh` — the sibling plugin directory, no separate lookup rule for this one call. **A missing script is not a failure here: skip this step in silence and let the run end as it stands.** The script swallows its own write errors and exits 0 even then, so nothing branches on its code either. This skill writes no code; it must not start failing over a line it could not write about itself.
|
|
|
|
| status | This skill's case |
|
|
| --- | --- |
|
|
| `ok` | All six groups returned and the merged list reached the caller, the snapshot is deleted and the lock is clear. A `merge-findings.sh` exit 1 is `ok` too: six groups read the diff and none had anything to report, which is a real answer |
|
|
| `blocked` | The review scope never existed, so no sub agent ran: step 1's `git diff` refused — not a repository, an unknown base revision, or an unreadable object. Nothing was reviewed and nothing could be |
|
|
| `degraded` | The report is handed over but the close-out is short: the step 1 snapshot is still on disk, or `write-guard.sh release` did not clear `$JSC_HOME/sessions/{sid}.lastskill` and the lock lingers until `JSC_WRITE_GUARD_TTL` |
|
|
| `failed` | The six groups ran and their findings never reached a report: `merge-findings.sh` exit 2, or a second exit 3 with malformed rows still on the table |
|
|
| `aborted` | There was nothing to review — the step 1 snapshot came back empty, so the diff holds no changed line, the file was deleted and no sub agent was spawned. Keep this apart from the `ok` above: both end in 「無發現」, and only the event tells a clean review from a review that never had a subject |
|
|
|
|
`{exit}` is the exit code of whatever decided the status, `0` for `ok`. `{detail}` is one short line well under 200 characters: changed-file and finding counts plus exit codes, never file paths, code excerpts, branch names, or personal data.
|
|
|
|
Completion condition: the command has run, or the script was absent and this step was skipped.
|
|
|
|
## Notes
|
|
|
|
- `jsc-hooks/hooks/comment-scope.sh` already matches the pattern-detectable comment scope items after every file write. Group 2 covers what patterns cannot decide — project code names, customer names, and context-dependent review traces — plus the overall judgment; the two never report the same finding twice.
|
|
- **Removed protection, on purpose.** Group 2 used to re-scan the pattern-detectable items as well, which made it the last net whenever the hook was not running: `JSC_COMMENT_SCOPE=off` turns `comment-scope.sh` off entirely, and a CLI with no `comment-scope.sh` wiring never runs it at all. That net is gone. On such a run, an issue id, a wiki page id, a work package id, or a commit hash left in a comment now reaches the commit with nothing catching it. Run `jsc-review:comment-cleanup` before commit whenever the hook is off or unwired.
|
|
- If group 5 examples are fetched from a database, they must be de-identified; never include personal data.
|
|
- When there are no findings, report the literal 「無發現」 explicitly; never leave the report empty.
|