feat(knowledge): sk knowledge bulk-tag — batch retag entries by query/filter (#722)

[magicpro97]

Summary

Implements sk knowledge bulk-tag command for batch retagging knowledge entries.

Closes #722

Changes

• knowledge-health.py: Added cmd_bulk_tag() with selector/mutation pipeline
• sk.py: Added bulk-tag routing to _GROUPS["knowledge"]
• test_fixes.py: 28 I722 tests (now correctly placed after sys.exit fix)

Fixes

• Ruff CI errors fixed (rebased on main b0352a5)
• I722 tests were dead code (after sys.exit) — fixed

Review

• Dead code bug found and fixed: 28 I722 tests now actually execute and pass

[Copilot]

Pull request overview

Adds a new sk knowledge bulk-tag subcommand that selects knowledge entries by query/filters and applies batch mutations (dry-run by default, --apply to commit), wiring it through sk.py and adding a corresponding integration-style test block in test_fixes.py.

Changes:

• Add cmd_bulk_tag() implementation and route "bulk-tag" in knowledge-health.py.
• Route sk knowledge bulk-tag ... via _GROUPS["knowledge"] in sk.py.
• Add I722 tests covering selectors, mutations, dry-run vs apply behavior, and basic routing/source checks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
knowledge-health.py	Implements cmd_bulk_tag() and adds main() routing for the bulk-tag subcommand.
sk.py	Adds grouped subcommand mapping and dispatch logic for sk knowledge bulk-tag.
test_fixes.py	Adds I722 test coverage for the new command and its behaviors.

[magicpro97]

Code Review — bulk-tag

SQL Injection / Parameterization ✅

All SQL uses ? placeholders throughout. The WHERE clauses are built from hardcoded strings; only values go into params. No f-string SQL, no string interpolation of user input into queries. Clean.

sk.py routing ✅

bulk-tag is correctly registered in _GROUPS["knowledge"] and dispatched with ["bulk-tag"] + sub_rest. Matches the pattern used by pin/unpin/pins.

Test coverage ✅

28 I722 tests cover: selector/mutation validation, dry-run no-op, --apply writes, idempotent --add-tag, combined mutations, --tag join filter, source="bulk-tag" check, sk.py route check, and f-string SQL absence check. Good.

---

🐛 Bug 1 — LIKE wildcards unescaped in --query (real correctness bug)

File: knowledge-health.py, lines 103–105

where.append("(ke.title LIKE ? OR ke.content LIKE ?)")
like_val = "%" + query_text + "%"  # BUG: % and _ in query_text act as wildcards
params.extend([like_val, like_val])

SQLite LIKE treats % (any chars) and _ (any single char) as metacharacters. They are not escaped before being embedded in the LIKE pattern. Examples of broken behavior:

• --query "%" → like_val = "%%" → matches every non-deleted entry
• --query "50%" → matches entries starting with "50", not those containing the literal "50%"
• --query "some_thing" → _ matches any single char, so "some-thing", "some.thing" etc. all match

Because bulk-tag is a data-modifying command (mutates wing/room/tags in bulk), an unexpectedly broad selector is a correctness defect — not just cosmetic.

Fix: escape LIKE metacharacters before wrapping with %:

# Escape SQLite LIKE metacharacters in the user-supplied substring
_escaped = query_text.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
like_val = "%" + _escaped + "%"
where.append("(ke.title LIKE ? ESCAPE '\\' OR ke.content LIKE ? ESCAPE '\\')")

---

⚠️ Minor — DB connection not closed in exception path

File: knowledge-health.py, exception handler (after the mutation loop)

except Exception as exc:
    print(f"⚠ bulk-tag failed: {exc}", file=sys.stderr)
    sys.exit(1)   # db.close() never called

If get_db() succeeds but a later operation raises (e.g., db.commit() fails on full disk), db.close() is skipped. In a CLI the OS reclaims the handle on exit, so this is benign in practice, but for WAL-mode databases a reader lock can linger until GC. Recommend adding db.close() before sys.exit(1).

---

Both issues are in cmd_bulk_tag only. Everything else — SQL parameterization, routing, test coverage — looks correct.

[magicpro97]

Fixed all reviewer issues:

• LIKE metachar escaping: query_text now escapes \, %, _ with ESCAPE '\\' clause to prevent wildcard injection
• deleted_at column guard: Now uses PRAGMA table_info to check if column exists before adding it to WHERE clause — works on older DB schemas
• Connection leak: Added db.close() in the except handler with its own try/except to ensure cleanup on error

Requesting re-review. CI pending.