---
phase: 67-expansion-sql-coverage-and-tech-debt-cleanup
plan: 03
type: execute
wave: 2
depends_on: []
files_modified:
  - .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md
  - test/integration/test_adbc_queries.py
autonomous: true
requirements: []
must_haves:
  truths:
    - "C1: The WR-02 entry in `.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md` is reclassified from `skipped: needs-design-decision` to `not-a-defect` with explicit citation of CONTEXT.md D-08 / D-09 (Phase 66 CONTEXT.md: ADBC tests are catalog-resolution regression guards, NOT value-correctness tests). The frontmatter status flips from `partial` to `complete` (or `final`) and `skipped: 1` flips to `skipped: 0`."
    - "C2: The WR-01 entry in `.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md` retains its sentinel-value fix but gains an inline rationale paragraph documenting the design call (per CONTEXT.md D-13): the original `COUNT(*) == 2` already discriminated routing-vs-fallback; sentinels make routing failures fail faster and more diagnostically; the `value test` objection is technically present but minor — sentinels are smoke values, not domain assertions. No code change in this task — documentation only."
    - "C3: `test/integration/test_adbc_queries.py` line 97 (or wherever `FORCE INSTALL '{extension_path}'` currently lives) is hardened against single-quote injection. Two acceptable forms: (a) `shlex.quote(str(extension_path))` substituted into the f-string, OR (b) `str(extension_path).replace(\"'\", \"''\")` per SQL-escape convention. Project-internal helper today; low practical risk; one-line hardening."
    - "Each item is a single-edit atomic commit per CONTEXT.md `<format>` (`<type>(67): <subject>` conventional commit pattern, e.g. `docs(67): reclassify WR-02 as not-a-defect per D-08/D-09`)."
    - "`just test-all` exits 0 (C3 touches a test file, so the full quality gate is required; C1+C2 are documentation-only and do not affect tests but they share the plan)."
  artifacts:
    - path: ".planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md"
      provides: "Updated WR-02 entry (reclassified to not-a-defect with D-08/D-09 citation) + updated WR-01 entry (sentinel-keep rationale documented)"
      contains: "not-a-defect"
    - path: "test/integration/test_adbc_queries.py"
      provides: "FORCE INSTALL hardened against single-quote injection at the path-interpolation site"
      contains: "shlex.quote"
  key_links:
    - from: "66-REVIEW-FIX.md WR-02 entry"
      to: "66-CONTEXT.md D-08, D-09"
      via: "explicit citation in the not-a-defect rationale"
      pattern: "D-08|D-09"
    - from: "66-REVIEW-FIX.md WR-01 entry"
      to: "67-CONTEXT.md D-13"
      via: "inline design-call rationale block"
      pattern: "sentinel"
    - from: "test/integration/test_adbc_queries.py FORCE INSTALL site"
      to: "shlex.quote OR escape replacement"
      via: "one-line hardening at the f-string substitution"
      pattern: "shlex.quote|replace\\(\"'\""
---

<objective>
Three bookkeeping items from the Phase 66 review-pass that are individual-commit-sized and don't fit Plan 01 (test-coverage) or Plan 02 (body parser). All three are mechanical:
- C1 reclassifies a single review entry's disposition (documentation).
- C2 documents a sentinel-keep design call (documentation).
- C3 wraps a one-line `FORCE INSTALL` interpolation in `shlex.quote`.

Purpose per CONTEXT.md D-12..D-14: these are tiny items that surfaced during the Phase 66 code-review-fix pass; they touch files Phase 66 already changed and each is single-edit-sized. Folding them into a single plan keeps each commit atomic without spawning three near-empty plans.

Output:
- C1: WR-02 disposition flipped in `66-REVIEW-FIX.md`
- C2: WR-01 rationale block appended in `66-REVIEW-FIX.md`
- C3: `FORCE INSTALL` site hardened in `test/integration/test_adbc_queries.py`
- Three atomic commits, one per item
- `just test-all` green after C3 (quality gate triggered by the Python test file edit)
</objective>

<execution_context>
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
@$HOME/.claude/get-shit-done/templates/summary.md
</execution_context>

<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/67-expansion-sql-coverage-and-tech-debt-cleanup/67-CONTEXT.md
@.planning/phases/66-expansion-qualification-adbc-tests/66-CONTEXT.md
@.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW.md
@.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md

# C3 surgery site
@test/integration/test_adbc_queries.py

# Project rules — Rules 1 & 2 non-negotiable; pre-commit fmt+clippy still applies (but only to .rs)
@CLAUDE.md
</context>

<tasks>

<task type="auto">
  <name>Task 1 (C1): Reclassify WR-02 disposition in 66-REVIEW-FIX.md from skipped:needs-design-decision to not-a-defect</name>
  <files>.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md</files>
  <read_first>
    - .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md (the full file — locate the WR-02 entry currently at lines 116-153 under `## Skipped Issues`; note the frontmatter at the top where `findings_in_scope: 6`, `fixed: 5`, `skipped: 1`, `status: partial` will need updating)
    - .planning/phases/66-expansion-qualification-adbc-tests/66-CONTEXT.md (specifically D-08 and D-09 — the framing that ADBC tests are catalog-resolution regression guards, not value-correctness tests; this is the citation C1 anchors to)
    - .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW.md (lines 53-58, the original WR-02 finding — confirm the wording you're countering)
  </read_first>
  <action>
1. Update the frontmatter at the top of `66-REVIEW-FIX.md`:
   - `fixed: 5` → `fixed: 5` (no change to count of code-affecting fixes; C1 is a disposition reclassification, not a new code fix)
   - `skipped: 1` → `skipped: 0`
   - `status: partial` → `status: complete`
   - Add a new field: `reclassified_post_phase66: 1` (or analogous; mirror existing key style). If the schema doesn't accept arbitrary keys, just note the reclassification in the prose `Summary` block instead.
   - Update the `Summary` block accordingly to read something like: "Findings in scope: 6 (all Warning-tier). Fixed: 5 (WR-01, WR-03, WR-04, WR-05, WR-06). Skipped at original fix time: 1 (WR-02). Reclassified in Phase 67 Plan 03 Task 1 to `not-a-defect` per CONTEXT.md D-08/D-09."

2. Locate the WR-02 entry under `## Skipped Issues` (line ~116). Either:
   - **Option A (preferred)**: MOVE the entry from `## Skipped Issues` to a new section `## Reclassified Post-Phase-66 (not a defect)` placed AFTER the existing `## Fixed Issues` section, BEFORE `## Skipped Issues` (which becomes empty and can be removed or left with a "(empty after Plan 03 reclassification)" note).
   - **Option B**: Leave the entry in place and prepend a `**STATUS UPDATE (Phase 67 Plan 03):** Reclassified to not-a-defect per CONTEXT.md D-08/D-09 — see rationale below.` line.

3. Append a rationale block to the WR-02 entry. The block must explicitly cite CONTEXT.md D-08 and D-09:

   ```
   **Disposition (post-Phase-66 reclassification):** `not-a-defect`.

   **Rationale:** Per Phase 66 CONTEXT.md D-08 and D-09, the ADBC integration tests in `test_adbc_queries.py` are catalog-resolution regression guards, not value-correctness tests. Their purpose is to catch the failure class `Catalog Error: Table with name X does not exist!` arising from unqualified `quote_table_ref` emission on a per-call Connection whose default catalog/schema search path diverges from `memory.main`. Count-only assertions are sufficient for that purpose: if a regression breaks qualified emission, the count-only assertion fails with a Catalog Error before the count is even computed; if a regression returns wrong rows with right cardinality (the case WR-02 worries about), that is a value-correctness regression and belongs to the SQL-layer fixture set, not the ADBC catalog-resolution guard.

   The value-correctness regression surface that WR-02 names is covered (or will be) by `test/sql/phase67_qualified_emission.test` (Phase 67 Plan 01 A1) and the existing fixture-level behavioural tests in `phase47_semi_additive.test` and `phase48_window_metrics.test`. Per the project's stated testing strategy ("primarily test for expected re-written SQL", see SCOPE.md), value-correctness lives at the sqllogictest layer not the ADBC layer.

   Reclassified in Phase 67 Plan 03 Task 1 (commit <SHA placeholder>).
   ```

4. Do NOT delete the original `**Reason: needs-design-decision**` body or the `**Why skipped:**` reasoning — preserve them as historical context; the new disposition block is additive.

5. Commit message convention: `docs(67): reclassify WR-02 as not-a-defect per Phase 66 D-08/D-09`.
  </action>
  <verify>
    <automated>grep -c "not-a-defect" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md && grep -c "D-08\|D-09" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md && grep -c "skipped: 0\|status: complete" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md</automated>
  </verify>
  <acceptance_criteria>
    - `grep -c "not-a-defect" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md` returns >= 1.
    - The frontmatter shows `skipped: 0` (or analogous indicating zero remaining skipped findings).
    - The WR-02 rationale block explicitly references both `D-08` and `D-09` (citation anchors).
    - The original `needs-design-decision` body is preserved (`grep -c "needs-design-decision" 66-REVIEW-FIX.md` returns >= 1 — historical content kept).
    - This task is committed atomically with the message `docs(67): reclassify WR-02 as not-a-defect per Phase 66 D-08/D-09`.
  </acceptance_criteria>
  <done>WR-02 reclassified; CONTEXT.md D-08/D-09 cited; frontmatter updated; atomic commit landed.</done>
</task>

<task type="auto">
  <name>Task 2 (C2): Document WR-01 sentinel-keep rationale in 66-REVIEW-FIX.md</name>
  <files>.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md</files>
  <read_first>
    - .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md (the WR-01 entry around line 85-113, currently marked as `requires human verification`)
    - 67-CONTEXT.md D-13 (the design call wording — "keep the sentinel-value fix as-is; the original `COUNT(*) == 2` already discriminated routing-vs-fallback; sentinels make routing failures fail faster and more diagnostically; document the rationale so the design call is recoverable")
  </read_first>
  <action>
1. Append a rationale block to the WR-01 entry in `66-REVIEW-FIX.md`. The block records the design call from CONTEXT.md D-13 so any future reader (or maintainer revisiting the test) can recover the reasoning without re-deriving it:

   ```
   **Sentinel-keep design call (Phase 67 Plan 03 Task 2):**

   The sentinel-value fix landed in commit `7669878` is retained. The trade-off considered:

   - The original `COUNT(*) == 2` assertion already discriminated routing-vs-fallback in scenario 6: raw expansion over the source `sales` table produces 3 rows (`US`, `EU`, `APAC` — see scenario setup), so a silent fall-back to raw expansion would have failed the count check.
   - The sentinel values `('US', -1.00), ('EU', -2.00)` (replacing the original `('US', 100.00), ('EU', 200.00)` that coincide with `SUM(s.amount) GROUP BY region`) make a routing failure fail FASTER (on the scalar value check) and more DIAGNOSTICALLY (the test name + assertion message immediately surface the value mismatch rather than the count discrepancy).
   - The "value test" objection is technically present (the assertion now compares a specific scalar, not just a row count), but minor in context: sentinels are smoke values chosen to be impossible-from-raw-expansion, not domain assertions about correct materialization arithmetic. They function as canaries, not as value-correctness tests.
   - The cleaner alternative (revert to `COUNT(*) == 2` with the 3-row source) loses the failure-mode-precision benefit and gains nothing. The split-the-difference alternative (one sentinel for smoke, then count) over-engineers a single-scenario test.

   Design call recorded so the call is recoverable. No code change in this task; the sentinel fix from commit `7669878` stands.
   ```

2. Do NOT modify any code in `test_adbc_queries.py` for this task. C2 is documentation-only.

3. Do NOT remove or rewrite the original `**Applied fix: requires human verification**` body — the new block is additive.

4. Commit message convention: `docs(67): document WR-01 sentinel-keep design call per D-13`.
  </action>
  <verify>
    <automated>grep -c "Sentinel-keep design call" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md && grep -c "7669878" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md</automated>
  </verify>
  <acceptance_criteria>
    - `grep -c "Sentinel-keep design call" .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md` returns >= 1.
    - The commit SHA `7669878` (the sentinel-fix commit per MEMORY.md) is referenced in the rationale.
    - The original WR-01 fix body is preserved.
    - No `.py` files are modified in this task (verifiable via `git diff --name-only HEAD~1` showing only the markdown file).
    - Atomic commit: `docs(67): document WR-01 sentinel-keep design call per D-13`.
  </acceptance_criteria>
  <done>WR-01 sentinel-keep rationale documented; design call recoverable; atomic commit landed; no code changes.</done>
</task>

<task type="auto">
  <name>Task 3 (C3): Harden FORCE INSTALL path interpolation in test_adbc_queries.py against quote injection</name>
  <files>test/integration/test_adbc_queries.py</files>
  <read_first>
    - test/integration/test_adbc_queries.py (lines 95-100 — the `_bootstrap_extension` function defining `FORCE INSTALL '{extension_path}'` and `LOAD '{extension_path}'`; verify both interpolations need the same hardening or only the FORCE INSTALL one)
    - .planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW.md (lines 113-117 — the IN-02 finding wording; this confirms only the FORCE INSTALL site is flagged, but a sibling LOAD site at the same function with identical interpolation has the same surface)
    - 67-CONTEXT.md D-14 (the specific instruction to wrap with `shlex.quote` or equivalent)
  </read_first>
  <action>
1. Read `_bootstrap_extension` (around lines 95-100):
   ```python
   def _bootstrap_extension(conn, extension_path: Path) -> None:
       _execute(conn, f"FORCE INSTALL '{extension_path}'")
       # ... and possibly _execute(conn, f"LOAD '{extension_path}'")
   ```

2. Choose ONE hardening approach (D-14 lists `shlex.quote` OR equivalent escaping):

   **Approach A — `shlex.quote` (preferred, idiomatic Python):**
   ```python
   import shlex  # add to imports at top of file if absent
   # ...
   def _bootstrap_extension(conn, extension_path: Path) -> None:
       quoted = shlex.quote(str(extension_path))
       _execute(conn, f"FORCE INSTALL {quoted}")
       # Note: shlex.quote wraps with single quotes itself, so drop the literal ' ' in the f-string
   ```
   D-14 framing: CONTEXT.md D-14 reads "shlex.quote or equivalent escaping" — the choice is the executor's. `shlex.quote` is the idiomatic Python helper but uses POSIX shell-quoting semantics. SQL-literal escape (doubling single quotes) is the equivalent escaping under D-14 for an interpolation that lands inside a SQL string literal: both forms close the injection surface; they differ in which target grammar's escape rules they apply. We choose SQL-escape below because the interpolation target IS a SQL string literal, not a shell argument.

   **Approach B — SQL-correct escape (recommended for this site):**
   ```python
   def _bootstrap_extension(conn, extension_path: Path) -> None:
       path_str = str(extension_path).replace("'", "''")
       _execute(conn, f"FORCE INSTALL '{path_str}'")
       # ... apply the same hardening at any sibling LOAD '{path}' interpolation
   ```
   Per IN-02 fix suggestion in `66-REVIEW.md:117`: `extension_path_sql = str(extension_path).replace("'", "''")`. This is the SQL-string-literal escape: doubling single quotes is the DuckDB-recognised escape inside a single-quoted string literal. This approach is correct for both POSIX paths AND any path containing a single quote.

3. **Apply Approach B** (SQL-correct escape). Both forms satisfy D-14's "shlex.quote OR equivalent escaping" clause; SQL-escape is the equivalent escaping for a SQL string literal interpolation, which is the target grammar here.

4. If there is a sibling `LOAD '{extension_path}'` interpolation in the same `_bootstrap_extension` function, apply the SAME hardening. Both interpolations enter SQL strings.

5. Add a short comment above the hardening block explaining why: `# IN-02: escape single-quotes for SQL string literal (path may contain ' though project-internal paths today don't)`.

6. Do NOT touch the `test_adbc_transactions.py` mirror — out of scope per CONTEXT.md (`66-REVIEW.md:116` explicitly says "out of scope to fix here"). If a sibling test file uses identical interpolation, surface it in the SUMMARY as a follow-up TECH-DEBT candidate; do NOT fix in this task.

7. Commit message convention: `fix(67): IN-02 escape extension_path for SQL string literal in test_adbc_queries`.

8. Run the test to confirm no regression:
   - `just test-adbc-queries > /tmp/claude/test_adbc_67_03_task3.log 2>&1; RC=$?; tail -80 /tmp/claude/test_adbc_67_03_task3.log; test "$RC" -eq 0`
   - Expected: 7/7 PASS, 0 FAIL. The hardening should be a no-op for the project-internal path that does not contain a single quote.

9. Run the full quality gate at the end of this task (since C3 touches a test file):
   - `just test-all > /tmp/claude/test_all_67_03_task3.log 2>&1`
   - Both MUST exit 0.

10. If `mktemp` is blocked at any step, retry with `dangerouslyDisableSandbox: true` per CLAUDE.md Rule 2.
  </action>
  <verify>
    <automated>grep -n "replace(\"'\", \"''\")" test/integration/test_adbc_queries.py > /tmp/claude/escape_check_67_03_task3.log; cat /tmp/claude/escape_check_67_03_task3.log; just test-adbc-queries > /tmp/claude/test_adbc_67_03_task3.log 2>&1; RC=$?; tail -80 /tmp/claude/test_adbc_67_03_task3.log; test "$RC" -eq 0 && just test-all > /tmp/claude/test_all_67_03_task3.log 2>&1; RC=$?; tail -120 /tmp/claude/test_all_67_03_task3.log; test "$RC" -eq 0</automated>
  </verify>
  <acceptance_criteria>
    - `grep -c "replace(\"'\", \"''\")" test/integration/test_adbc_queries.py` returns >= 1 (the SQL-escape was applied).
    - `grep -c "IN-02" test/integration/test_adbc_queries.py` returns >= 1 (the explanatory comment with the finding ID).
    - `just test-adbc-queries` exits 0 with 7/7 PASS / 0 FAIL.
    - `just test-all` exits 0.
    - `test/integration/test_adbc_transactions.py` was NOT modified (out of scope per CONTEXT.md).
    - Atomic commit: `fix(67): IN-02 escape extension_path for SQL string literal in test_adbc_queries`.
  </acceptance_criteria>
  <done>FORCE INSTALL (and any sibling LOAD) interpolation hardened with SQL-correct escape; 7/7 ADBC queries green; full test-all green; atomic commit landed.</done>
</task>

</tasks>

<threat_model>
## Trust Boundaries

| Boundary | Description |
|----------|-------------|
| `extension_path` → SQL string literal in `_bootstrap_extension` | Path crosses from Python file-system query into a DuckDB SQL string. The path is project-internal (from `get_extension_path()`), so the threat is theoretical today; C3 closes the surface before it becomes a real defect. |

## STRIDE Threat Register

| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-67-03-01 | Tampering | `_bootstrap_extension` path interpolation | mitigate | Apply SQL-string-literal escape (`replace("'", "''")`) at the interpolation site. Closes the IN-02 finding. |
| T-67-03-02 | Tampering | `66-REVIEW-FIX.md` markdown content | accept | Doc-only file; checked into repo; tampering surfaces in code review. C1 + C2 are documentation reclassifications, no production impact. |

C3 closes an existing low-severity IN-02 finding (project-internal helper, low practical risk per CONTEXT.md D-14). No new high-severity threats. C1 and C2 are documentation-only and introduce no threat surface.
</threat_model>

<verification>
- C1: `66-REVIEW-FIX.md` frontmatter reflects the WR-02 reclassification (`skipped: 0` or analogous; `status: complete`); WR-02 rationale block cites D-08 and D-09 explicitly.
- C2: `66-REVIEW-FIX.md` WR-01 entry contains the sentinel-keep design-call rationale referencing commit `7669878`.
- C3: `test/integration/test_adbc_queries.py` `_bootstrap_extension` applies `replace("'", "''")` to `extension_path` before interpolating into the SQL string literal.
- `just test-adbc-queries` exits 0 with 7/7 PASS / 0 FAIL (no regression from C3).
- `just test-all` exits 0 (full quality gate, since C3 touches a test file).
- Three atomic commits land sequentially with conventional commit messages.
- `test_adbc_transactions.py` is NOT modified (out-of-scope mirror).
- Tail-buffer rule honored: all build/test commands redirect to `/tmp/claude/*.log` first.
- Sandbox bypass used only when `mktemp` blocks build/test, per CLAUDE.md Rule 2.
</verification>

<success_criteria>
- C1 disposition reclassified with D-08/D-09 citation; frontmatter updated.
- C2 sentinel-keep rationale documented inline; commit `7669878` cited.
- C3 SQL-escape applied at the FORCE INSTALL (and any sibling LOAD) interpolation; IN-02 comment present.
- Three atomic commits.
- `just test-all` exits 0.
- No code changes outside `test/integration/test_adbc_queries.py` and `.planning/phases/66-expansion-qualification-adbc-tests/66-REVIEW-FIX.md`.
- No release-prep tasks included.
</success_criteria>

<output>
Create `.planning/phases/67-expansion-sql-coverage-and-tech-debt-cleanup/67-03-SUMMARY.md` when done. Include:
- The three commit SHAs (one per item) and their messages
- The exact text of the WR-02 reclassification rationale (verbatim block) and the WR-01 sentinel-keep rationale (verbatim block)
- The diff snippet showing the C3 escape change (before/after)
- Confirmation that `just test-adbc-queries` and `just test-all` both exited 0
- Whether `test_adbc_transactions.py` was identified as having an analogous IN-02 surface — surface as TECH-DEBT-followup candidate if so
</output>
