---
phase: 07-workflow-smoke-convention-locks
reviewed: 2026-05-24T00:00:00Z
depth: standard
files_reviewed: 14
files_reviewed_list:
  - docs/adr/0009-ldtk-gridsize-convention.md
  - docs/adr/0010-ldtk-tileset-source-hash.md
  - docs/adr/0011-bncentral-gridvania-chunking.md
  - docs/adr/0012-ldtk-version-pin.md
  - tools/ldtk-version.txt
  - traceable-reqs.toml
  - tools/scripts/lint-no-req-placeholders.mjs
  - tools/scripts/lint-atlas-hash.mjs
  - tools/scripts/check-conversion-regression.mjs
  - tooling/no-inline-origin.ts
  - package.json
  - docs/deploy/LOCAL-DEPLOY.md
  - .gitignore
  - .planning/phases/07-workflow-smoke-convention-locks/SMOKE-NOTES.md
findings:
  critical: 0
  warning: 5
  info: 6
  total: 11
status: issues_found
---

# Phase 7: Code Review Report

**Reviewed:** 2026-05-24
**Depth:** standard
**Files Reviewed:** 14
**Status:** issues_found (non-blocking)

## Summary

Phase 7 ships 4 ADRs (0009-0012), 3 new `.mjs` lint/check scripts (one active drift-lock, two Phase-8/Phase-9 stubs), wires a `pnpm preflight` chain in `package.json`, adds a MANDATORY preflight gate section to `LOCAL-DEPLOY.md`, and captures an operator UAT for the synthetic 8000x6400 GridVania smoke test.

Production code surface is intentionally minimal. The three new scripts use only static, hardcoded inputs in their `execSync` calls — no shell injection vectors are reachable from operator-supplied data. ADR cross-references all resolve to extant ADR files (0004, 0008, 0009, 0010, 0011, 0012). Live execution of `lint-no-req-placeholders`, `lint-atlas-hash`, and `check-conversion-regression` confirms each exits 0 against the current tree.

The findings below are all WARNING or INFO. Nothing blocks the phase from shipping. The most material concerns are (a) the `lint-atlas-hash.mjs` `_smoke` filter being substring-rather-than-segment matched, (b) the placeholder allowlist's path-prefix coarseness creating a soft bypass when authoring inside allowlisted directories, and (c) the preflight chain's documented "continue on trace:check failure" carve-out partially undermining the MANDATORY framing.

## Warnings

### WR-01: `lint-atlas-hash.mjs` `_smoke` filter is substring-matched, not path-segment-matched

**File:** `tools/scripts/lint-atlas-hash.mjs:35`
**Issue:**
```js
const ldtkFiles = findLdtk(MAPS_DIR).filter((p) => !p.includes('_smoke'));
```
`.includes('_smoke')` matches the substring anywhere in the joined path. A legitimate production map named `maps/foo_smoketest.ldtk`, `maps/_smoke_backup/real.ldtk`, or `maps/world-with-_smoke-in-the-name.ldtk` would be silently excluded from the Phase 8 hash check once activated. The intent (per `.gitignore` and SMOKE-NOTES) is to exclude only `maps/_smoke/*.ldtk` — i.e., files under the `_smoke` directory segment.

**Fix:**
```js
import { sep } from 'node:path';
// ...
const ldtkFiles = findLdtk(MAPS_DIR).filter(
  (p) => !p.split(sep).includes('_smoke'),
);
```
This matches the directory name as a whole path segment, not a substring of any segment. Cross-platform-safe (handles both `\` and `/` via `path.sep` — and for already-joined paths the regex `/(^|[\\/])_smoke([\\/]|$)/` would also work).

---

### WR-02: `lint-no-req-placeholders.mjs` allowlist is path-prefix-only — new files under any allowlisted prefix bypass the lint entirely

**File:** `tools/scripts/lint-no-req-placeholders.mjs:29-40`
**Issue:** The allowlist matches by path prefix (entries ending in `/`) or exact path. Any new `.md` file authored under `.planning/milestones/`, `.planning/research/v1.1/`, or `.planning/phases/07-workflow-smoke-convention-locks/` is unconditionally allowed to contain `REQ-...-XX` placeholders regardless of intent. A future operator can drop `.planning/milestones/foo.md` with arbitrary placeholder text and the lint will not fire. This is by design (planning docs need to discuss the placeholder shape) but it does mean the "drift-lock" is enforced primarily on `apps/`, `packages/`, `tools/`, and the non-allowlisted parts of `docs/`/`.planning/` — not across the whole repo. Worth documenting in the script preamble so future readers understand the enforcement boundary.

To the user's specific question — yes, the allowlist is also bypassable by *moving* a placeholder-containing file *into* an allowlisted prefix (e.g., creating `.planning/milestones/2026-q3.md` to hold what was previously in a flagged file). The script does not inspect file content for "is this a legitimate documentation-of-the-pattern context vs an accidental real placeholder"; it trusts the prefix.

**Fix:** Either (a) tighten the allowlist to exact file paths only (no `/`-suffix prefixes) so each new exception is an explicit single-line allowlist edit reviewable in PR, or (b) document the prefix-trust posture in the script preamble:

```js
// ALLOWLIST POSTURE: path-prefix entries trust every descendant. Adding
// a new placeholder-quoting doc INSIDE `.planning/milestones/` or
// `.planning/research/v1.1/` does NOT trip the lint. Drift detection
// is scoped to (a) non-allowlisted paths in scan roots, (b) source
// code under apps/, packages/, tools/.
```

(a) is the stricter posture; (b) accepts the gap but makes it visible.

---

### WR-03: `lint-atlas-hash.mjs` `findLdtk` recursion can infinite-loop on symlink cycles and crashes on broken symlinks

**File:** `tools/scripts/lint-atlas-hash.mjs:25-33`
**Issue:**
```js
function findLdtk(dir, out = []) {
  for (const f of readdirSync(dir)) {
    const p = join(dir, f);
    const s = statSync(p);   // follows symlinks
    if (s.isDirectory()) findLdtk(p, out);
    ...
  }
}
```
`statSync` follows symlinks. If `maps/` ever contains a symlink to itself or a cycle (e.g., `maps/a -> .`), `findLdtk` recurses infinitely until stack overflow. More immediately likely: a broken symlink (e.g., a deleted target) causes `statSync` to throw `ENOENT` and crashes the entire preflight chain with no actionable message. The script also has no try/catch around the directory walk.

**Fix:**
```js
import { readdirSync, existsSync, lstatSync } from 'node:fs';
// ...
function findLdtk(dir, out = []) {
  for (const f of readdirSync(dir)) {
    const p = join(dir, f);
    let s;
    try { s = lstatSync(p); } catch { continue; }  // skip broken symlinks
    if (s.isSymbolicLink()) continue;              // don't follow symlinks at all
    if (s.isDirectory()) findLdtk(p, out);
    else if (p.endsWith('.ldtk')) out.push(p);
  }
  return out;
}
```
Or use `readdirSync(dir, { withFileTypes: true, recursive: true })` (Node 20+) which is symlink-safe by default.

---

### WR-04: Preflight gate documented as MANDATORY in LOCAL-DEPLOY.md, but the runbook's earlier "Pre-flight" section permits continuing past a failed `pnpm trace:check`

**File:** `docs/deploy/LOCAL-DEPLOY.md:33-37` vs `:38-52`
**Issue:** The "Pre-flight" section (lines 33-37) says of `pnpm trace:check`:
> "If it fails, confirm the deploy-touched requirements ... are not newly regressed, record the pre-existing findings, and continue only for staging UAT redeploys."

Twenty lines later, the "Preflight gate (MANDATORY)" section makes `pnpm preflight` (which includes `pnpm trace:check` as step 4) a hard prerequisite: "MUST exit 0 before any flyctl deploy". These two stances are in direct tension. An operator reading top-to-bottom will hit the carve-out first ("continue on failure for staging UAT redeploys") and then be told the gate is mandatory. In practice the MANDATORY framing is undermined by the prior paragraph.

**Fix:** Either delete the "If it fails, ..." carve-out from the Pre-flight section now that the MANDATORY gate is in place, or explicitly note in the MANDATORY section that the carve-out is superseded:

> "Note: the carve-out above for pre-existing `pnpm trace:check` findings is **superseded** by this MANDATORY gate. Any `pnpm preflight` failure aborts deploy regardless of whether the finding is newly regressed."

The latter is the documented operator intent per CONTEXT.md Phase 7 D-14 ("first failure aborts the chain"). Without this resolution the runbook says two opposite things.

---

### WR-05: `check-conversion-regression.mjs` stub always exits 0 — no forcing function if Phase 9 implementation is forgotten

**File:** `tools/scripts/check-conversion-regression.mjs:12-13`
**Issue:** The stub silently no-ops with `process.exit(0)`. There is no surface that flags "this script needs to be replaced by Phase 9" beyond the `TODO Phase 9` comment. By contrast `lint-atlas-hash.mjs:42-44` includes an annoying-enough-to-notice log when production `.ldtk` files exist — a deliberate forcing function. `check-conversion-regression.mjs` has no equivalent guard for the analogous Phase 9 trigger condition (e.g., `tools/room-converter/` containing an `ldtk-import` subcommand).

**Fix:** Add a Phase 9 readiness check that nudges noisily when the precondition is met:
```js
import { existsSync } from 'node:fs';
const LDTK_IMPORT_HINT = 'tools/room-converter/src/commands/ldtk-import.ts';
if (existsSync(LDTK_IMPORT_HINT)) {
  console.warn(
    `check-conversion-regression: WARN — ldtk-import subcommand exists (${LDTK_IMPORT_HINT}) but regression check is still a stub. Phase 9 activation overdue.`,
  );
}
console.log('check-conversion-regression: skipped (pending Phase 9 ldtk-import subcommand)');
process.exit(0);
```
Non-failing but visible, matching the `lint-atlas-hash.mjs` PITFALL 4 posture.

---

## Info

### IN-01: `lint-no-req-placeholders.mjs` allowlist omits `.planning/research/` (only `v1.1/` subdirectory is allowed)

**File:** `tools/scripts/lint-no-req-placeholders.mjs:34`
**Issue:** The allowlist entry is `.planning/research/v1.1/` — strictly the v1.1 subdirectory. The parent `.planning/research/` (containing the original `STACK.md`, `FEATURES.md`, `PITFALLS.md`, `ARCHITECTURE.md`, `SUMMARY.md`) is NOT covered. Today those files do not contain the placeholder pattern (verified — `git grep` returns no hits outside `v1.1/`), but a future edit could add one. Robustness suggestion: extend the allowlist to `.planning/research/` (covers all milestones' research dirs).

**Fix:**
```js
'.planning/research/',
```
in place of `.planning/research/v1.1/`.

---

### IN-02: `lint-atlas-hash.mjs` no try/catch on `readdirSync(MAPS_DIR)` at top-level

**File:** `tools/scripts/lint-atlas-hash.mjs:35`
**Issue:** If `maps/` exists but is unreadable (permissions), the script crashes with an uncaught `EACCES`. Preflight chain aborts with no actionable message. Minor — operator can read the OS error — but a guarded message would be nicer.

**Fix:** Wrap `findLdtk(MAPS_DIR)` in try/catch with a clearer error like `lint-atlas-hash: ERROR — cannot read maps/ (check permissions): <e.message>`.

---

### IN-03: Inline `node -e` preflight step 1 has no error message when `tools/ldtk-version.txt` is missing

**File:** `package.json:57`
**Issue:**
```
"preflight": "node -e \"const v=require('fs').readFileSync('tools/ldtk-version.txt','utf8').trim();if(!v)throw new Error('ldtk-version.txt empty');console.log('LDtk pin:',v)\" && ..."
```
The `readFileSync` throws ENOENT with stock Node error wording when the file doesn't exist (`Error: ENOENT: no such file or directory, open 'tools/ldtk-version.txt'`). For an operator unfamiliar with the convention, a wrapped error mentioning ADR 0012 would be friendlier. The empty-string case is handled (`'ldtk-version.txt empty'`); the missing-file case is not wrapped.

**Fix:** Extract to a tiny script (`tools/scripts/check-ldtk-pin.mjs`) with a clearer error trail, or extend the inline node to try/catch the read:
```js
let v;
try { v = require('fs').readFileSync('tools/ldtk-version.txt','utf8').trim(); }
catch(e) { throw new Error('tools/ldtk-version.txt missing or unreadable — see ADR 0012'); }
if (!v) throw new Error('tools/ldtk-version.txt empty — see ADR 0012');
console.log('LDtk pin:', v);
```
This is borderline (ADR 0012 explicitly elects the 3-line inline form as a Positive consequence) — accept-as-is is also reasonable.

---

### IN-04: `lint-no-req-placeholders.mjs` `git grep` command does not pass `--no-pager` or `-z`

**File:** `tools/scripts/lint-no-req-placeholders.mjs:58`
**Issue:** With `execSync` + `stdio: ['ignore', 'pipe', 'pipe']`, pager invocation is suppressed automatically (no TTY), so this is currently moot. However, a future refactor that pipes through a TTY-attached parent would surface this. Filenames containing newlines (rare but valid on Unix) would also break the line-splitter at line 63. Defensive: `-z` for NUL-separated records is the cleanest fix; or simpler, `--no-pager`. Cosmetic.

**Fix:** None required for current behavior. If hardening later: `git --no-pager grep -nE ...` or use `-z` and split on `\0`.

---

### IN-05: SMOKE-NOTES.md states 134 MB `.ldtk` after partial paint — flagged for v1.2 but worth confirming `_smoke/` `.gitignore` rule was effective

**File:** `.planning/phases/07-workflow-smoke-convention-locks/SMOKE-NOTES.md:53` + `.gitignore:75`
**Issue:** `.gitignore` excludes `maps/_smoke/*.ldtk`. A `git status --porcelain` at smoke-test time should show no untracked `.ldtk` blob. The phase-end commit must not have inadvertently committed the 134 MB file. Verification: `git ls-files maps/_smoke/` should return empty. (Not run here, but the runbook should verify before phase close.)

**Fix:** Add a one-line check to the phase verification step:
```bash
test -z "$(git ls-files maps/_smoke/ 2>/dev/null)" || { echo 'FAIL: _smoke .ldtk leaked into git history'; exit 1; }
```

---

### IN-06: ADR 0009 "Alternatives considered" `44 / 40` rejection cites "non-square cells with no path to entity sub-tile placement" — minor wording

**File:** `docs/adr/0009-ldtk-gridsize-convention.md:108-112`
**Issue:** The phrasing "would require non-square cells with no path to entity sub-tile placement, and LDtk's data model doesn't support it" conflates two reasons (non-square cells; data model limitation). LDtk's `gridSize` is fundamentally a single integer per layer — that's the data-model constraint and it alone is dispositive. The "non-square cells" framing implies LDtk could accept a width/height pair if it weren't for cell-shape concerns. Tightening to "LDtk `gridSize` is a single integer per layer (not a width × height pair); 44 ≠ 40 so no single integer satisfies both axes" makes the rejection sharper. Cosmetic.

**Fix:** Tighten the wording per above. Non-blocking.

---

## Cross-cutting notes (not findings)

- **ADR cross-references** all resolve to extant files: ADR 0009 → 0008 (canvas-base-resolution.md exists), ADR 0010 → 0004 (room-hot-reload.md exists), ADR 0011 → 0009, ADR 0012 → 0009/0010/0011. No dangling links.
- **Shell injection** in the three new `.mjs` scripts: not reachable. All `execSync` inputs (`BAD_PATTERN`, `SCAN_PATHS`) are hardcoded compile-time constants; no user input flows in. `tooling/no-inline-origin.ts` (unchanged behaviorally, only tag added) is the same posture.
- **Path traversal** in `lint-atlas-hash.mjs`: contained to the `maps/` constant directory; no user-controlled path component.
- **`pnpm preflight` `&&` chain on Windows**: pnpm spawns scripts via the OS default shell (`cmd.exe` on Windows). `&&` is supported in `cmd.exe`, `bash`, `zsh`, and PowerShell 7+. Lockstep aborts on first non-zero exit — verified by inspection of the chain.
- **`traceable-reqs.toml`** tightened `REQ-HYG-02` to `required_stages = ["doc", "impl"]`. The script `tools/scripts/lint-no-req-placeholders.mjs` carries `[impl->REQ-HYG-02]` — the impl stage is satisfied. Trace coverage holds.
- **Live execution**: `lint-no-req-placeholders`, `lint-atlas-hash`, `check-conversion-regression` all exit 0 against the current tree (verified during review).

---

_Reviewed: 2026-05-24_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_