fix(review): 補上註解腳本路徑與缺漏的失敗分支
- 註解範圍腳本的路徑少一層目錄,照著寫會找不到檔案,補上 hooks 那一層。 - 建置測試跑完卻失敗時沒有分支可走,只寫得出「通過」。現在要指出指令、結束碼,並判斷失敗是不是本次改動造成的。 - 取得差異失敗時原本會安靜跳過,現在原樣帶出錯誤並停手。判不出範圍不等於沒有東西要審。 - 禁止清單原本抄了兩份,改成只指向註解範圍的正本,兩份不會各自演化。 - 第二組不再重掃樣式判得出來的項目。hook 關掉或沒接上時就沒有最後一道網,這件事寫進技能文件,不讓它默默消失。 - 差異只算一次,落成一份快照檔給六組共讀;檔名帶執行代號,同一台機器平行跑不會互相覆蓋。註解清理改成一個檔案一個 sub agent 平行跑。
This commit is contained in:
@@ -20,26 +20,44 @@ Review changed code against `references/smells.md` (from the book *Refactoring*)
|
|||||||
|
|
||||||
## Steps
|
## Steps
|
||||||
|
|
||||||
1. Get the review scope: `git diff` (uncommitted changes) or `git diff {base}...HEAD` (against the base branch when an implementation is complete); list the changed files. An empty diff → report the literal 「無發現」 and stop here; spawn no sub agent. Completion condition: the changed-file list is non-empty and reported, or the run already ended with 「無發現」.
|
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:
|
2. Review in six groups, and every group **MUST run as a sub agent**; the six groups may run in parallel:
|
||||||
|
|
||||||
| Group | Scope |
|
| Group | Scope |
|
||||||
| --- | --- |
|
| --- | --- |
|
||||||
| 1 Bloaters | smells.md group 1 |
|
| 1 Bloaters | smells.md group 1 |
|
||||||
| 2 Obscurity | smells.md group 2, including 2.5 comment scope leaks — comments carrying issue ids, wiki page ids, work package ids, commit hashes, @ mentions, external document links, review rounds, finding ids, reviewer aliases, or review status labels; the full banned and allowed lists live in `references/comment-scope.md` |
|
| 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 |
|
| 3 Couplers | smells.md group 3 |
|
||||||
| 4 Dispensables & Others | smells.md group 4 |
|
| 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) |
|
| 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 |
|
| 6 Shallow Module | smells.md group 6 |
|
||||||
|
|
||||||
Instructions for each sub agent: read only, change nothing; check every changed line and its enclosing function or class against the group's definitions and detection signals in smells.md; report each finding as `file:line`, smell name, severity (高、中、低 per the smells.md scale), one sentence of evidence, and the suggested refactoring. Findings are reported in Traditional Chinese.
|
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.
|
||||||
|
|
||||||
Completion condition: all six groups have returned — a group with nothing to report still returns 「無發現」 for its group.
|
Each sub agent returns its findings as TSV rows — one finding per row, six fields separated by a single tab, in this order:
|
||||||
3. Merge the six groups' findings: deduplicate (when one location hits several groups, merge and list every smell), then sort by severity. Start this step only once all six groups have returned; a group still running means the merge waits. Completion condition: every finding appears exactly once in the merged list, ordered 高 → 中 → 低.
|
|
||||||
4. Report the finding list. **This skill never modifies code**; the caller decides what to fix (inside the implementation flow, 高 and 中 are normally mandatory, 低 is judgment). Completion condition: the report is handed to the caller and the fix decision is left to them.
|
| 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.
|
||||||
|
|
||||||
## Notes
|
## Notes
|
||||||
|
|
||||||
- `jsc-hooks`' `comment-scope.sh` already matches the pattern-detectable items after every file write. Group 2 here 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.
|
- `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.
|
- 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.
|
- When there are no findings, report the literal 「無發現」 explicitly; never leave the report empty.
|
||||||
|
|||||||
@@ -19,15 +19,15 @@ Use `references/comment-scope.md` for the banned list, allowed list, and rewrite
|
|||||||
|
|
||||||
## Division of Labor
|
## Division of Labor
|
||||||
|
|
||||||
- `jsc-hooks/comment-scope.sh` blocks pattern-detectable violations after writes.
|
- `jsc-hooks/hooks/comment-scope.sh` blocks pattern-detectable violations after writes.
|
||||||
- `jsc-review:code-review` group 2 reports judgment-based cases that patterns cannot decide.
|
- `jsc-review:code-review` group 2 reports judgment-based cases that patterns cannot decide.
|
||||||
- This skill cleans the changed comments. Do not report the same finding again when a hook or code review already reported it; either clean it or explain why it is outside this skill's scope.
|
- This skill cleans the changed comments. Do not report the same finding again when a hook or code review already reported it; either clean it or explain why it is outside this skill's scope.
|
||||||
|
|
||||||
## Steps
|
## Steps
|
||||||
|
|
||||||
1. Define the scope: inspect the current diff and list only added or modified comment lines. Include untouched legacy comments only when the user explicitly requested that. Completion condition: the cleanup scope is listed by file, or the run ends with the literal 「無發現」.
|
1. Define the scope with `tools/changed-comments.sh`, run from the repository being cleaned: no argument for uncommitted work, `tools/changed-comments.sh {base}` when an implementation is complete and the base branch is the comparison point. It prints one row per changed comment line — file, line number, content — so this step never judges by eye which lines are comments; that judgment lives in the script, next to the one `jsc-hooks/hooks/comment-scope.sh` already uses. Read its exit code: `0` its stdout is the cleanup scope, and step 2 hands each file its own rows; `1` this change added or modified no comment line, so report the literal 「無發現」 and stop, spawn no sub agent; `2` a parameter or environment error, so report the script's stderr verbatim and stop, spawn no sub agent, and never substitute a hand-read diff — a scope that could not be computed is not an empty scope. Widen to untouched legacy comments only when the user explicitly asked for that, and say so in the report. Completion condition: the cleanup scope is listed by file, or the run ended with 「無發現」 or with the script's error.
|
||||||
2. Compare each scoped comment with `references/comment-scope.md`. Remove process-only details and keep the real reason for the code. If a whole comment is only process detail and no real reason remains, delete the whole comment. Completion condition: every scoped comment is either unchanged with a reason, rewritten, or deleted.
|
2. Rewrite one file per sub agent, and every file **MUST run as a sub agent**. One file's comments never depend on another file's, so the sub agents run in parallel. Each sub agent is handed one file path and that file's scoped comment lines from step 1, and it compares each of them with `references/comment-scope.md`, removes process-only details, and keeps the real reason for the code. A comment that is only process detail with no real reason left is deleted whole. Each sub agent returns the file path, the categories it removed, and the line numbers it touched. Completion condition: every file's sub agent has returned, and every scoped comment is either unchanged with a stated reason, rewritten, or deleted.
|
||||||
3. Re-read the changed area after every rewrite. Confirm the sentence is complete, the logic still reads naturally, and no dangling fragment remains after deletion. Completion condition: every touched comment reads as a complete explanation or is gone.
|
3. Each sub agent re-reads its own changed area before it returns. It confirms every sentence is complete, the logic still reads naturally, and no dangling fragment remains after a deletion; a fragment it cannot resolve is reported instead of left behind. Completion condition: every sub agent has confirmed its file, and no returned report names an unresolved fragment.
|
||||||
4. Change comments and documentation strings only. Do not change executable behavior, identifiers, control flow, data shape, or tests except when a test fixture literally asserts the old comment text. Completion condition: `git diff` shows comment-only or documentation-string-only edits.
|
4. Change comments and documentation strings only. Do not change executable behavior, identifiers, control flow, data shape, or tests except when a test fixture literally asserts the old comment text. `jsc-hooks/hooks/write-guard.sh` in `review` mode is the code-level backstop, but it covers this skill only as far as jsc-hooks can decide a comment line precisely: where that decision is not precise, the guard is limited to `jsc-review:code-review` and `jsc-review:api-doc`, which write nothing at all, and this skill runs unguarded. Only claude has a `PreToolUse` hook in the first place, so on codex, copilot, antigravity, and kiro this step's prose is the only constraint. Completion condition: `git diff` shows comment-only or documentation-string-only edits.
|
||||||
5. Run the smallest relevant build or test command for the changed project. If no project command is available, run syntax checks for touched scripts and report the gap. Completion condition: verification passed, or the exact missing command is reported.
|
5. Run the smallest relevant build or test command once for the whole changed project, after every sub agent in step 2 has returned. Read the exit code. `0` — verification passed. Non-zero — the command ran and failed, so name the command, its exit code, and the failing output, then decide whether this run caused it: a failure that names a file this run touched is treated as caused here, so restore that file's comment syntax and re-run the command once; if it fails the same way again, revert this run's edits in that file and report the revert. A failure that names no file this run touched is reported as pre-existing, and the cleanup edits stay. If no project command is available, run syntax checks for the touched scripts and report the gap. Completion condition: the command exited `0`, or the report states the command, its exit code and whether the failure belongs to this run, or the exact missing command is reported.
|
||||||
6. Report the cleanup by category, not by full diff. Completion condition: the report names which categories were removed, which files were touched, and whether verification passed.
|
6. Report the cleanup by category, not by full diff. Completion condition: the report names which categories were removed, which files were touched, and whether verification passed.
|
||||||
|
|||||||
Reference in New Issue
Block a user