Skip to content

Commit e8f5f06

Browse files
jakeclaude
andcommitted
fix(kb): address code-review findings on the auto-routing hook
Adversarial multi-agent review of the kb-mod-cacg branch surfaced these (all verified against the real kb binary + the test suite): - SIGPIPE/exit-5 abort: extracting the prompt via `... | head -c 2000` under pipefail aborted the hook on >64KB prompts (141) and on malformed stdin (5). Now extract with `|| true` and truncate in-shell (char-based, no mid-UTF8 split). - No time bound: wrap `kb search` in run_with_timeout (repo convention); the existing `|| exit 0` already turns a 124 into a clean no-op. - Flag-like prompts: pass the query after `--` so a prompt starting with '-' is not misparsed by clap and dropped. - Context-injection hardening: scrub newlines from .title/.path/.card_id (not just .summary) so a tampered card cannot forge a heading in additionalContext. - Injected hint now points at `.claude/knowledge/kb-query.sh show <card_id>` (resolved from the discovered KB root) instead of bare `kb show` (which needs --source-matrix and would error if run literally). - kb_top_k now clamps to [1,25] so the injected block stays bounded at source. - Codex prompts: qualify the `kb verify --round-summary` command as requiring a verify-tier export (the default query tier omits chunks_manifest.json). - Tests (16 -> 27): mock kb now logs argv + is query-sensitive, so the suite asserts the prompt->search plumbing (verb, query-after-`--`, --source-matrix/ --summaries/--top-k wiring); adds malformed-stdin (exit 0), >64KB prompt (no SIGPIPE), top-k clamp, and kb_discover_root/kb_resolve_binary override cases. test-kb-provenance-format.sh stays 25/25; config-merge + bitlesson green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 197c3c5 commit e8f5f06

5 files changed

Lines changed: 120 additions & 37 deletions

File tree

hooks/kb-knowledge-route.sh

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ PLUGIN_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
2424
source "$SCRIPT_DIR/lib/project-root.sh"
2525
source "$PLUGIN_ROOT/scripts/lib/config-loader.sh"
2626
source "$PLUGIN_ROOT/scripts/lib/kb-route-lib.sh"
27+
source "$PLUGIN_ROOT/scripts/portable-timeout.sh" # run_with_timeout (falls back to direct exec if unavailable)
2728

2829
# jq is mandatory for both config parsing and result shaping; without it, no-op.
2930
command -v jq >/dev/null 2>&1 || exit 0
@@ -51,31 +52,38 @@ KB_BIN="$(kb_resolve_binary "$MERGED")" || exit 0
5152

5253
SM="$KB_ROOT/out/$DECK/source_matrix.json"
5354
SUM="$KB_ROOT/out/$DECK/summaries.json"
55+
# Project-relative path to the exported query wrapper (falls back to absolute if KB is outside the project).
56+
QUERY_REL="${KB_ROOT#"$PROJECT_ROOT"/}/kb-query.sh"
5457

55-
# Extract the user prompt; cap the query length so a huge paste stays a sane query.
56-
PROMPT="$(printf '%s' "$INPUT" | jq -r '.prompt // empty' 2>/dev/null | head -c 2000)"
58+
# Extract the user prompt and cap its length. Truncate in-shell (char-based) rather than
59+
# piping to `head -c`: under `set -o pipefail` a >64KB prompt SIGPIPE-kills the pipeline
60+
# (exit 141) and `jq` on malformed stdin exits 5 — both would abort the hook instead of
61+
# no-op'ing. `|| true` + ${var:0:N} keeps every path graceful and avoids mid-UTF8 byte splits.
62+
PROMPT="$(printf '%s' "$INPUT" | jq -r '.prompt // empty' 2>/dev/null || true)"
63+
PROMPT="${PROMPT:0:2000}"
5764
[[ -n "${PROMPT//[[:space:]]/}" ]] || exit 0
5865

59-
# Deterministic search. Failure (bad manifest, version drift, etc.) -> no-op.
60-
HITS="$("$KB_BIN" search "$PROMPT" --json --source-matrix "$SM" --summaries "$SUM" --top-k "$TOP_K" 2>/dev/null)" || exit 0
66+
# Deterministic search, time-bounded (a wedged/huge corpus must not hang the prompt). `--`
67+
# ends option parsing so a prompt beginning with '-' is taken as the QUERY, not a flag.
68+
HITS="$(run_with_timeout "${KB_SEARCH_TIMEOUT:-10}" "$KB_BIN" search --json --source-matrix "$SM" --summaries "$SUM" --top-k "$TOP_K" -- "$PROMPT" 2>/dev/null)" || exit 0
6169
printf '%s' "$HITS" | jq -e 'type == "array" and length > 0' >/dev/null 2>&1 || exit 0
6270

6371
# Render the suggestion block. The framing is load-bearing: these are SUGGESTIONS,
6472
# not provenance. `kb verify --round-summary` treats the `## Knowledge Consulted`
6573
# section as authoritative, so this block must not be copied wholesale into it.
66-
BLOCK="$(printf '%s' "$HITS" | jq -r --arg deck "$DECK" '
74+
BLOCK="$(printf '%s' "$HITS" | jq -r --arg deck "$DECK" --arg wrapper "$QUERY_REL" '
6775
"## CACG Knowledge — auto-routed matches (deck: \($deck))\n"
68-
+ "Search suggestions only. Open a card with `kb show <card_id>` (or read its `.md`) and cite it in your round summary'"'"'s `## Knowledge Consulted` section ONLY if you actually opened and used it.\n\n"
76+
+ "Search suggestions only. Open a card with `\($wrapper) show <card_id>` (or read its `.md`) and cite it in your round summary'"'"'s `## Knowledge Consulted` section ONLY if you actually opened and used it.\n\n"
6977
+ ( [ .[]
70-
| "- `\(.path)` (\(.card_id)) — \(.title): "
78+
| "- `\((.path // "") | gsub("[\r\n]+";" "))` (\((.card_id // "") | gsub("[\r\n]+";" "))) — \((.title // "") | gsub("[\r\n]+";" ") | .[0:200]): "
7179
+ ((.summary // "") | gsub("[\r\n]+"; " ") | .[0:200])
7280
] | join("\n") )
7381
')"
7482

7583
# Hard byte cap so a pathological corpus can never bloat every prompt.
7684
MAX_BYTES=4096
7785
if [[ "$(printf '%s' "$BLOCK" | wc -c)" -gt "$MAX_BYTES" ]]; then
78-
BLOCK="$(printf '%s' "$BLOCK" | head -c "$MAX_BYTES")"$'\n- … (truncated)'
86+
BLOCK="$(printf '%s' "$BLOCK" | head -c "$MAX_BYTES" || true)"$'\n- … (truncated)'
7987
fi
8088

8189
jq -n --arg ctx "$BLOCK" \

prompt-template/codex/full-alignment-review.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ The `Mainline Progress Verdict` line is mandatory. If you omit it, the Humanize
7474
- Verify Claude's claims match reality
7575
- Identify any gaps, bugs, or incomplete work
7676
- Reference @{{DOCS_PATH}} for design documents
77-
- **Knowledge Provenance Check**: Verify Claude's summary includes a `## Knowledge Consulted` section listing concrete reference files (e.g. CACG cards under `.claude/knowledge/cards/<deck>/**/*.md`, other `.claude/knowledge/**/*.md`, vendor manuals, paper PDFs, metric manifests) or the exact string `N/A -- task not KB-relevant this round`. If the section is missing on KB-relevant work, or if `N/A` is used as a shortcut for domain-specific / modeling / metrics tasks, flag as a Blocking Side Issue. When a CACG export is present, the cited card paths can be content-verified with `kb verify --round-summary <summary-file> --source-matrix .claude/knowledge/out/<deck>/source_matrix.json --chunks-manifest .claude/knowledge/out/<deck>/chunks_manifest.json`.
77+
- **Knowledge Provenance Check**: Verify Claude's summary includes a `## Knowledge Consulted` section listing concrete reference files (e.g. CACG cards under `.claude/knowledge/cards/<deck>/**/*.md`, other `.claude/knowledge/**/*.md`, vendor manuals, paper PDFs, metric manifests) or the exact string `N/A -- task not KB-relevant this round`. If the section is missing on KB-relevant work, or if `N/A` is used as a shortcut for domain-specific / modeling / metrics tasks, flag as a Blocking Side Issue. When a **verify-tier (or full)** CACG export is present (`chunks_manifest.json` ships only at `--tier verify` and above; the default `query` tier omits it), the cited card paths can be content-verified with `kb verify --round-summary <summary-file> --source-matrix .claude/knowledge/out/<deck>/source_matrix.json --chunks-manifest .claude/knowledge/out/<deck>/chunks_manifest.json`.
7878

7979
## Part 4: {{GOAL_TRACKER_UPDATE_SECTION}}
8080

prompt-template/codex/regular-review.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ Below is Claude's summary of the work completed:
3535
- Your review should elaborate on those unfinished tasks, explore the codebase, and draft an implementation plan.
3636
- A good engineering implementation plan should be **singular, directive, and definitive**, rather than discussing multiple possible implementation options.
3737
- The implementation plan should be **unambiguous**, internally consistent, and coherent from beginning to end, so that **Claude can execute the work accurately and without error**.
38-
- **Knowledge Provenance Check**: Verify Claude's summary includes a `## Knowledge Consulted` section listing concrete reference files (e.g. CACG cards under `.claude/knowledge/cards/<deck>/**/*.md`, other `.claude/knowledge/**/*.md`, vendor manuals, paper PDFs, metric manifests) or the exact string `N/A -- task not KB-relevant this round`. If the section is missing on KB-relevant work, or if `N/A` is used as a shortcut for domain-specific / modeling / metrics tasks, flag as a Blocking Side Issue. When a CACG export is present, the cited card paths can be content-verified with `kb verify --round-summary <summary-file> --source-matrix .claude/knowledge/out/<deck>/source_matrix.json --chunks-manifest .claude/knowledge/out/<deck>/chunks_manifest.json`.
38+
- **Knowledge Provenance Check**: Verify Claude's summary includes a `## Knowledge Consulted` section listing concrete reference files (e.g. CACG cards under `.claude/knowledge/cards/<deck>/**/*.md`, other `.claude/knowledge/**/*.md`, vendor manuals, paper PDFs, metric manifests) or the exact string `N/A -- task not KB-relevant this round`. If the section is missing on KB-relevant work, or if `N/A` is used as a shortcut for domain-specific / modeling / metrics tasks, flag as a Blocking Side Issue. When a **verify-tier (or full)** CACG export is present (`chunks_manifest.json` ships only at `--tier verify` and above; the default `query` tier omits it), the cited card paths can be content-verified with `kb verify --round-summary <summary-file> --source-matrix .claude/knowledge/out/<deck>/source_matrix.json --chunks-manifest .claude/knowledge/out/<deck>/chunks_manifest.json`.
3939

4040
## Part 2: Goal Alignment Check (MANDATORY)
4141

scripts/lib/kb-route-lib.sh

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,15 @@ kb_resolve_binary() {
6161
command -v "$bin" >/dev/null 2>&1
6262
}
6363

64-
# kb_sanitize_top_k <raw> -> prints a positive integer (default 5)
64+
# kb_sanitize_top_k <raw> -> prints a positive integer in [1,25] (default 5)
6565
# get_config_value tostring-coerces numbers, so kb_top_k arrives as a
66-
# string. Reject 0/negative/non-numeric and fall back to 5.
66+
# string. Reject 0/negative/non-numeric (-> 5) and clamp huge values (-> 25)
67+
# so the injected block stays bounded at the source.
6768
kb_sanitize_top_k() {
6869
local raw="$1"
69-
if [[ "$raw" =~ ^[1-9][0-9]*$ ]]; then printf '%s' "$raw"; else printf '5'; fi
70+
if [[ "$raw" =~ ^[1-9][0-9]*$ ]]; then
71+
if [ "$raw" -gt 25 ]; then printf '25'; else printf '%s' "$raw"; fi
72+
else
73+
printf '5'
74+
fi
7075
}

0 commit comments

Comments
 (0)