Replace local spec file loading with issue-body fetching

[dcellison]

Summary

• Remove filesystem-based spec resolution (resolve_spec_from_body, resolve_spec_from_branch, load_spec) which had a path traversal vulnerability (spec: ../../etc/kai/env could read arbitrary files) and exposed internal directory structure in PR bodies
• Add load_spec_from_issue() which extracts fixes #N / closes #N / resolves #N references from the PR description and fetches the linked issue bodies via the GitHub API
• Multiple issue references are fetched and concatenated with clear separators
• Remove SPEC_DIR env var, spec_dir config field, and all associated passthrough in webhook.py and install.py
• Update CONTRIBUTING.md to reference issue-based spec checking instead of file paths
• Replace ~190 lines of spec resolution tests with tests for the new approach

Fixes #87

Test plan

• Regex matches all GitHub closing keywords (fixes/fixed/fix, closes/closed/close, resolves/resolved/resolve)
• Case-insensitive matching
• Multiple issue refs extracted and deduplicated
• Cross-repo refs (owner/repo#N) not matched
• Old spec: marker is inert (not matched)
• Happy path: issue body fetched and returned with header
• No issue link: returns None, review proceeds without spec
• Empty/None description: returns None
• API failure: logs warning, returns None (graceful degradation)
• Multiple issues: bodies concatenated with separators
• Duplicate refs: fetched only once
• Partial failure: successful fetches returned, failures skipped
• Empty issue body: skipped
• All 899 tests pass

[dcellison]

Review by Kai

PR Review: Replace local spec file loading with issue-body fetching

Overall this is a clean, well-motivated change. The security fix (removing path traversal) is correct and the new approach is simpler. A few issues worth addressing:

---

Warning: gh --jq .body returns literal "null" for bodyless issues

review.py, load_spec_from_issue — when a GitHub issue has no body, the gh api call with --jq .body outputs the string null\n, not an empty string. The current guard:

body = stdout.decode().strip()
if body:
    specs.append(f"## Issue #{issue_num}\n\n{body}")

...passes for "null" since it's truthy. The spec would contain ## Issue #42\n\nnull, which is both misleading and noise in the review prompt.

Fix: Either change the check to if body and body != "null":, or pipe through --jq '.body // ""' to coerce null to empty string at the source.

The existing test test_empty_issue_body_skipped only covers b"" — add a b"null" case to catch this.

---

Suggestion: \s+ in regex matches newlines

review.py:120

r"\b(?:fix(?:e[sd])?|close[sd]?|resolve[sd]?)\s+#(\d+)\b"

\s+ includes \n, so "fix\n#42" would match even though GitHub only recognizes closing keywords on the same line as the issue reference. This could produce false positives if a PR body coincidentally has e.g. a word "fix" near a bare #N on the next line.

Fix: Use [ \t]+ or [^\S\n]+ to restrict to horizontal whitespace.

---

Suggestion: No timeout on proc.communicate()

review.py, load_spec_from_issue — there's no timeout on await proc.communicate(). A hung or slow gh invocation (network issues, rate limiting) blocks the review indefinitely, and the effect compounds with multiple issue references. Consider wrapping with asyncio.wait_for(..., timeout=10.0) inside the try/except.

---

Suggestion: Issue body content reaches the prompt unescaped

review.py — issue bodies are concatenated verbatim into the spec string, which then flows into build_review_prompt. If the downstream prompt uses XML delimiters for injection prevention (as the module docstring claims), an issue body containing the closing XML tag (e.g. </spec>) could break out of its container. Worth verifying that build_review_prompt properly escapes or neutralizes XML metacharacters within the spec block.

---

The SPEC_DIR removal, config cleanup, and test updates are all correct. The deduplication logic and partial-failure handling are solid.

[dcellison]

Review by Kai

PR Review: Replace local spec file loading with issue-body fetching

All four issues from the prior review have been addressed: .body // "" coercion, [^\S\n]+ to restrict whitespace, the 15-second timeout, and the prior XML injection concern was acknowledged but not addressed (author's call).

One new issue:

---

Warning: except TimeoutError misses asyncio.TimeoutError on Python < 3.11

review.py, load_spec_from_issue inner try/except

except TimeoutError:
    proc.kill()
    await proc.wait()

On Python 3.10, asyncio.wait_for raises asyncio.TimeoutError, which inherits from concurrent.futures.TimeoutError → Exception — not from the built-in TimeoutError. The bare except TimeoutError won't catch it. The outer except Exception will, so no crash occurs, but proc.kill() is skipped and the orphaned gh process keeps running.

Fix: except asyncio.TimeoutError works correctly across all Python versions.

The test also doesn't cover the real timeout path — it sets communicate.side_effect = TimeoutError() which makes the coroutine raise directly, whereas asyncio.wait_for cancels the task and raises its own asyncio.TimeoutError. The test passes today but doesn't catch the Python 3.10 mismatch.

---

Everything else looks clean: the deduplication logic, partial-failure handling, the regex (word boundaries, newline guard, cross-repo exclusion), the --jq '.body // ""' coercion, and the SPEC_DIR removal are all correct. The test coverage for TestIssueRefPattern and TestLoadSpecFromIssue is solid.

[dcellison]

Re: TimeoutError vs asyncio.TimeoutError - will not fix.

On Python 3.11+, asyncio.TimeoutError is an alias for the builtin TimeoutError (they are the same class). Kai runs Python 3.13. Ruff's UP041 rule actively rejects asyncio.TimeoutError in favor of the builtin, so using both would fail lint. The original code is correct for the target version.