fix(makefile): address Story 13.3 senior-developer review findings

CHANGELOG.md

@@ -7,8 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Plugin resolver review follow-ups (Story 13.3 senior-developer review):
+  - **H1**: malformed `.devrail.yml` no longer silently treated as
+    "no plugins declared". `yq` parse failures now surface as a structured
+    error event and exit 2 instead of being swallowed by `|| echo 0`.
+  - **M1**: lockfile verifier passes the `.devrail.yml` source URL via
+    `strenv()` instead of string-interpolating it into the yq query —
+    defends against malformed/malicious source values breaking the query.
+  - **M2**: slug collisions (two plugin sources with the same `basename`)
+    are now detected upfront and rejected with a clear "plugin slug
+    collision" error event. Previously the second plugin would silently
+    overwrite the first's cache.
+  - **M3**: `fetch_to_cache` now performs an atomic swap (move existing
+    target aside, install new, remove old) so concurrent `make check`
+    invocations during a `make plugins-update` see either the old tree
+    or the new one — never an absent or half-populated path.
+  - **M4**: `compute_content_hash` and `derive_slug` extracted to
+    `lib/plugin-cache.sh` and shared between resolver and verifier
+    (was duplicated; future drift risk eliminated).
+  - **M5**: resolver now invokes `plugin-validator.sh` on the fetched
+    manifest before computing content_hash. Authors hit manifest
+    structural issues at `make plugins-update` time, not at every
+    subsequent `make check`.
+  - **L1**: `fetch_to_cache` comment updated to reflect 4 args (not 3).
+  - **L2**: `derive_slug` strips the `.git` suffix so
+    `https://example.com/foo.git` produces cache path
+    `<plugins-dir>/foo/<rev>/`, not `<plugins-dir>/foo.git/<rev>/`.
+  - **L3**: lockfile entries written with double-quoted YAML scalars so
+    source URLs containing colons, brackets, or other reserved chars
+    don't break parsing.
+  - **L4**: smoke test now covers `_plugins-update` no-op when no
+    plugins declared (companion to existing `_plugins-verify` case).
+  - **L5**: idempotent-fetch test now asserts cache sentinel `.devrail.sha`
+    mtime is stable across re-runs (proves no re-clone happened).
+  - **L6**: smoke test exercises a `.git`-suffixed source URL.
+
 ### Added
 
+- `lib/plugin-cache.sh` — shared `derive_slug` and `compute_content_hash`
+  helpers used by the resolver and verifier. Single source of truth for
+  the on-disk cache layout.
+- `tests/test-plugin-resolver.sh` extended from 11 to 16 cases
+  (review-fix coverage: yq parse error, slug collision, `.git` suffix,
+  no-plugins update no-op, mtime-based idempotency).
 - Plugin resolver and lockfile (Story 13.3, Epic 13 / v1.10.x preview).
   - **`make plugins-update`** — public target that resolves every
     `.devrail.yml` plugin's `rev:` to an immutable SHA via `git ls-remote`,

lib/plugin-cache.sh

@@ -0,0 +1,68 @@
+#!/usr/bin/env bash
+# lib/plugin-cache.sh — Plugin cache helpers (Story 13.3)
+#
+# Purpose: Single source of truth for the plugin-cache slug derivation and
+#          content-hash computation. Sourced by both `plugin-resolver.sh`
+#          and `plugin-lockfile-verify.sh` so the two stay in lockstep.
+#
+# Usage:   source "${DEVRAIL_LIB}/plugin-cache.sh"
+# Dependencies: lib/log.sh
+#
+# Functions:
+#   derive_slug <source-url>          - Plugin cache directory slug
+#   compute_content_hash <dir>        - Deterministic sha256 of tree content
+
+# Guard against double-sourcing
+# shellcheck disable=SC2317
+if [[ -n "${_DEVRAIL_PLUGIN_CACHE_LOADED:-}" ]]; then
+  return 0 2>/dev/null || true
+fi
+readonly _DEVRAIL_PLUGIN_CACHE_LOADED=1
+
+# derive_slug prints the on-disk cache slug for a plugin source URL.
+#
+# - basename of the URL (so `https://github.com/foo/bar` → `bar`)
+# - strips a trailing `.git` suffix (so `bar.git` → `bar`)
+# - rejects empty input and slugs that contain shell-special characters
+#   (slashes, colons, etc.) — those would mean basename failed
+#
+# Example: derive_slug https://github.com/community/devrail-plugin-elixir.git
+#          → devrail-plugin-elixir
+derive_slug() {
+  local source_url="${1:-}"
+  if [[ -z "${source_url}" ]]; then
+    return 1
+  fi
+  local slug
+  slug="$(basename "${source_url}")"
+  # Strip optional .git suffix
+  slug="${slug%.git}"
+  # Reject anything that's not a typical project-name shape — defense against
+  # weird URLs that basename can't parse cleanly.
+  if [[ ! "${slug}" =~ ^[a-zA-Z0-9._-]+$ ]]; then
+    return 1
+  fi
+  printf "%s" "${slug}"
+}
+
+# compute_content_hash prints a sha256 hex digest over a directory's
+# non-.git/, non-sentinel files. Stable across machines via LC_ALL=C.
+# Returns non-zero if the directory doesn't exist.
+compute_content_hash() {
+  local dir="${1:-}"
+  if [[ ! -d "${dir}" ]]; then
+    if declare -f log_event >/dev/null 2>&1; then
+      log_event error "content_hash directory missing" path="${dir}" language=_plugins
+    fi
+    return 1
+  fi
+  (cd "${dir}" &&
+    LC_ALL=C find . -type f \
+      -not -path './.git/*' \
+      -not -name '.devrail.sha' \
+      -print0 |
+    LC_ALL=C sort -z |
+      xargs -0 sha256sum |
+      sha256sum |
+      cut -d' ' -f1)
+}

scripts/plugin-lockfile-verify.sh

@@ -10,7 +10,7 @@
 #          Exit 0 — verified or no-op (no plugins declared)
 #          Exit 2 — disagreement, missing lockfile, or tampering detected
 #
-# Dependencies: yq v4+, sha256sum, find, sort, lib/log.sh
+# Dependencies: yq v4+, sha256sum, find, sort, lib/log.sh, lib/plugin-cache.sh
 
 set -euo pipefail
 LC_ALL=C
@@ -22,6 +22,8 @@ DEVRAIL_LIB="${DEVRAIL_LIB:-${SCRIPT_DIR}/../lib}"
 
 # shellcheck source=../lib/log.sh
 source "${DEVRAIL_LIB}/log.sh"
+# shellcheck source=../lib/plugin-cache.sh
+source "${DEVRAIL_LIB}/plugin-cache.sh"
 
 # --- Help ---
 if [[ "${1:-}" == "--help" || "${1:-}" == "-h" ]]; then
@@ -43,11 +45,23 @@ fi
 
 require_cmd "yq" "yq is required (v4+)"
 
+# Distinguish "yq parse error" from "no plugins declared" (review fix H1).
+parse_plugin_count() {
+  local count
+  if ! count="$(yq -r '.plugins // [] | length' "${DEVRAIL_YML}" 2>&1)"; then
+    log_event error "config could not be parsed by yq" \
+      path="${DEVRAIL_YML}" reason="${count}" language=_plugins
+    return 1
+  fi
+  printf "%s" "${count}"
+}
+
+if ! plugin_count="$(parse_plugin_count)"; then
+  exit 2
+fi
+
 # --- No-op when no plugins declared (regression-safe for v1.9.x consumers) ---
-plugin_count="$(yq -r '.plugins // [] | length' "${DEVRAIL_YML}" 2>/dev/null || echo 0)"
 if [[ "${plugin_count}" == "0" ]]; then
-  # Even if .devrail.lock exists, having no plugins declared means there is
-  # nothing for the loader to load. Quietly succeed.
   exit 0
 fi
 
@@ -62,19 +76,6 @@ fi
 
 require_cmd "sha256sum" "sha256sum is required (coreutils)"
 
-# compute_content_hash <dir>
-compute_content_hash() {
-  local dir="$1"
-  (cd "${dir}" && find . -type f \
-    -not -path './.git/*' \
-    -not -name '.devrail.sha' \
-    -print0 |
-    sort -z |
-    xargs -0 sha256sum |
-    sha256sum |
-    cut -d' ' -f1)
-}
-
 # --- Cross-check every yml entry has a matching lock entry with same rev ---
 violations=0
 for i in $(seq 0 $((plugin_count - 1))); do
@@ -87,7 +88,10 @@ for i in $(seq 0 $((plugin_count - 1))); do
     continue
   fi
 
-  lock_rev="$(yq -r ".plugins[] | select(.source == \"${yml_source}\") | .rev" "${LOCKFILE}" 2>/dev/null | head -1)"
+  # Pass yml_source via env (strenv) so a malicious source URL with quotes,
+  # backslashes, or yq-expression-special characters can't break the query
+  # (review fix M1).
+  lock_rev="$(yml_source="${yml_source}" yq -r '.plugins[] | select(.source == strenv(yml_source)) | .rev' "${LOCKFILE}" 2>/dev/null | head -1)"
   if [[ -z "${lock_rev}" || "${lock_rev}" == "null" ]]; then
     log_event error "lockfile mismatch" \
       source="${yml_source}" \
@@ -106,12 +110,16 @@ for i in $(seq 0 $((plugin_count - 1))); do
     continue
   fi
 
-  # Content-hash tampering check: re-compute hash of cached tree, compare
-  # to the lockfile-recorded hash.
-  recorded_hash="$(yq -r ".plugins[] | select(.source == \"${yml_source}\") | .content_hash" "${LOCKFILE}" 2>/dev/null | head -1)"
+  # Content-hash tampering check.
+  recorded_hash="$(yml_source="${yml_source}" yq -r '.plugins[] | select(.source == strenv(yml_source)) | .content_hash' "${LOCKFILE}" 2>/dev/null | head -1)"
   recorded_hash="${recorded_hash#sha256:}"
 
-  slug="$(basename "${yml_source}")"
+  if ! slug="$(derive_slug "${yml_source}")"; then
+    log_event error "plugin source URL produced an invalid slug" \
+      source="${yml_source}" rev="${yml_rev}" language=_plugins
+    violations=$((violations + 1))
+    continue
+  fi
   cached_dir="${PLUGINS_DIR}/${slug}/${yml_rev}"
   if [[ ! -d "${cached_dir}" ]]; then
     log_event error "cached tree missing" \