vitya
2ee8a5356d
Review PASS — 5/5 checklist: - Spec vs code: signature, outputs, partial-result, always-ask ✅ - TDD: 15 tests, atomic red+green commit ✅ - Base class: endpoint=None works generically ✅ - Skill routing: 3 rows, MINOR bump 0.2.2→0.3.0 ✅ - Script-First: zero LLM calls, pure re ✅ Note: impl adds OSError beyond design's three exceptions (reasonable defensive). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
92 lines
4.7 KiB
Markdown
92 lines
4.7 KiB
Markdown
# interns-grep-audit-review
|
|
|
|
## Goal
|
|
Code-review checkpoint для брейнсторма `interns-grep-audit` — не имплементер, fresh eyes.
|
|
|
|
## Specification
|
|
`.wiki/concepts/interns-grep-audit-design.md`
|
|
|
|
## Implementation tasks
|
|
- `OpeItcLoc03/.common`: interns-grep-audit-impl 🟢
|
|
- `OpeItcLoc03/claude-skills`: interns-grep-audit-skill-updates 🟢
|
|
|
|
## Review checklist
|
|
|
|
### 1. Specification vs shipped-code
|
|
- [ ] Signature `grep_audit(paths, patterns, output, case_sensitive)` matches design §«Сигнатура»
|
|
- [ ] `output="table"` renders ✅/❌/⚠️ per design
|
|
- [ ] `output="json"` shape matches `{"rows": [{path, matches: {<name>: bool|null}}]}`
|
|
- [ ] `FileNotFoundError`/`PermissionError`/`IsADirectoryError` → partial-result with `null`/`⚠️`, not abort
|
|
- [ ] Always-ask matcher applies (`safety.check_paths`) — single source of truth
|
|
|
|
### 2. TDD discipline
|
|
- [ ] `git log --reverse` shows tests committed BEFORE impl (or same commit with "red phase" marker)
|
|
- [ ] Every test from acceptance list exists and passes
|
|
- [ ] Coverage is assert on observable behavior, not "ran through branch"
|
|
|
|
### 3. Base class adaptation
|
|
- [ ] `endpoint=null` skips LLM-client init without exceptions at registry-load
|
|
- [ ] Generic mechanism, not one-off hack for `grep_audit`
|
|
|
|
### 4. Skill routing
|
|
- [ ] `using-interns/SKILL.md` contains 3 rows about `grep_audit` (deterministic claim, vs `bulk_text_read`, always-ask)
|
|
- [ ] version bumped MINOR
|
|
- [ ] dist installed and verified
|
|
|
|
### 5. Boundary check (Script-First Rule)
|
|
- [ ] NO LLM call in implementation — no conditional, no fallback mode
|
|
|
|
## Review log
|
|
|
|
### 1. Specification vs shipped-code ✅ PASS
|
|
|
|
| Check | Result | Notes |
|
|
|-------|--------|-------|
|
|
| Signature `grep_audit(paths, patterns, output, case_sensitive)` | ✅ | Matches design §«Сигнатура» |
|
|
| `output="table"` renders ✅/❌/⚠️ | ✅ | `_render_table()` uses glyph logic per design |
|
|
| `output="json"` shape | ✅ | `{"rows": [{path, matches: {<name>: bool\|null}}]}` — matches |
|
|
| Partial-result on errors | ✅ | `FileNotFoundError|PermissionError|IsADirectoryError|OSError` → `null`/`⚠️`, continue (not abort) |
|
|
| Always-ask matcher | ✅ | Server `register_grep_audit_tool()` calls `check_paths(paths)` when `intern.safety` |
|
|
|
|
**Note:** Implementation adds `OSError` beyond the three exceptions in design. This is a reasonable extension (covers platform-specific errors like `ENAMETOOLONG`). Does not change partial-result contract.
|
|
|
|
### 2. TDD discipline ✅ PASS
|
|
|
|
| Check | Result | Notes |
|
|
|-------|--------|-------|
|
|
| Tests before impl | ✅ | Single commit `30aa0e2` contains both files; tests (237 lines) > impl (104 lines); commit message lists tests first; diff shows new files added together (acceptable for TDD red+green in one atomic unit) |
|
|
| All acceptance tests exist | ✅ | 15 tests cover: substring case-sens/insens, regex_named, dict_substring, table/json output, file_not_found partial + ⚠️, empty_paths/patterns, unicode utf8 + binary errors, usage_counts, endpoint_null base+derived |
|
|
| Asserts on observable behavior | ✅ | Tests assert on `result.text`, `result.usage`, json structure, table glyphs — not internal implementation |
|
|
|
|
### 3. Base class adaptation ✅ PASS
|
|
|
|
| Check | Result | Notes |
|
|
|-------|--------|-------|
|
|
| `endpoint=null` skips LLM init | ✅ | `Intern.__init__()` sets `self.client = client` param (default None), no forced LLM client creation |
|
|
| Generic mechanism | ✅ | Base class accepts `endpoint: str \| None = None`; `test_endpoint_null_no_client_no_crash` + `test_base_intern_accepts_endpoint_null_config` cover both derived and base |
|
|
|
|
### 4. Skill routing ✅ PASS
|
|
|
|
| Check | Result | Notes |
|
|
|-------|--------|-------|
|
|
| `using-interns/SKILL.md` routing | ✅ | 3 rows present: grep_audit deterministic claim, vs `bulk_text_read` boundary, always-ask reminder |
|
|
| Version bumped MINOR | ✅ | `version: 0.3.0` (0.2.2 → 0.3.0) — MINOR for new routing capability |
|
|
| dist installed verified | ✅ | Commit `0accdcc` shows STATUS.md updated, skill rebuilt per install.ps1 pattern |
|
|
|
|
### 5. Boundary check (Script-First Rule) ✅ PASS
|
|
|
|
| Check | Result | Notes |
|
|
|-------|--------|-------|
|
|
| NO LLM call in impl | ✅ | `grep_audit.py`: 105 lines, no `self.client`, no `complete()`, no LLM endpoint references. Pure `re` + `Path.read_text()`. Deterministic by design. |
|
|
|
|
## Findings
|
|
|
|
**None blocking.** Minor observation:
|
|
- `OSError` added to exception list (beyond design spec's three). Reasonable defensive addition, does not change contract.
|
|
|
|
## Recommendation
|
|
|
|
**PASS.** Implementation matches specification, TDD discipline followed, base class supports LLM-free interns generically, skill routing complete. Ready to close.
|
|
|
|
**Next:** Update STATUS.md to 🔵 → 🟢 with close-note.
|