Skip to content

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

Merged
matthew-on-git merged 1 commit intomainfrom
fix/13-3-review-followups
May 3, 2026
Merged

fix(makefile): address Story 13.3 senior-developer review findings#35
matthew-on-git merged 1 commit intomainfrom
fix/13-3-review-followups

Conversation

@matthew-on-git
Copy link
Copy Markdown
Contributor

@matthew-on-git matthew-on-git commented May 3, 2026

Summary

Adversarial review of Story 13.3 (PR #34, in flight) surfaced 12 findings: 1 HIGH, 5 MEDIUM, 6 LOW. All addressed here. Tests expanded from 11 to 16 cases.

This PR is based on feat/13-3-plugin-resolver (PR #34), not directly on main. Merge order: PR #34 first, then this PR.

Findings + fixes

🔴 HIGH

# Finding Fix
H1 Malformed .devrail.yml silently treated as "no plugins declared" — yq ... || echo 0 swallowed parse errors Both resolver and verifier wrap yq via a parse_plugin_count helper that distinguishes "parse failure" (exit 2, structured error) from "no plugins entry" (silent success).

🟡 MEDIUM

# Finding Fix
M1 Verifier interpolates source URL into yq expression unquoted — injection risk Pass via strenv() instead of string concatenation.
M2 Two sources with same basename collide on the same cache path Track SLUG_TO_SOURCE during the resolve loop; emit plugin slug collision error on the second occurrence.
M3 fetch_to_cache had rm -rf target; mv fetch_dir target race window Atomic swap: move existing target aside, install new, remove old (with rollback on failure). Concurrent readers always see either the old or the new tree.
M4 compute_content_hash duplicated between resolver and verifier Extracted to lib/plugin-cache.sh along with new derive_slug helper.
M5 Resolver didn't validate the fetched manifest — bad manifests failed at every make check Resolver now runs plugin-validator.sh on the fetched manifest before recording content_hash.

🟢 LOW

# Finding Fix
L1 fetch_to_cache comment said 3 args, takes 4 Comment fixed
L2 .git-suffixed URLs yielded ugly slugs (devrail-plugin-elixir.git) derive_slug strips .git
L3 Source URLs with colons written into YAML unquoted Lockfile values now double-quoted via yaml_quote helper
L4 No test for _plugins-update no-op with no plugins Added Case 15
L5 Idempotent-fetch test asserted event presence, not absence of re-clone Added Case 16 — checks .devrail.sha mtime stability
L6 No test for .git-suffixed source URL Added Case 14

Test results (local)

```
==> Case 1-16: all 16 plugin-resolver smoke checks pass (was 11)
==> tests/test-plugin-loader.sh: 11/11 (regression-safe)
==> tests/smoke-rails.sh: 4/4 (regression-safe)
==> make _check on dev-toolchain itself: pass
```

Behaviour change for consumers

No plugins: in .devrail.yml: zero change. Loader/resolver/verifier all no-op (same as PR #34).

Has plugins::

  • Malformed YAML now produces a clear error instead of "no plugins declared". (Behavior fix; impossible to depend on the old behavior in practice.)
  • .git-suffixed source URLs now resolve to a cleaner cache path (cache layouts that depended on the .git suffix would break — none exist in the wild yet because v1.10.0 is unreleased).
  • Lockfile format change: values are now double-quoted YAML scalars. Existing lockfiles continue to parse (yq accepts both forms); regenerated lockfiles will look slightly different in diffs.

Recommended next step

Merge PR #34 first, then this PR. After both merge, cut v1.10.2 patch release.

🤖 Generated with Claude Code

@matthew-on-git @claude
fix(makefile): address Story 13.3 senior-developer review findings
6b479ca 
Adversarial review of Story 13.3 surfaced 12 findings (1 HIGH, 5 MEDIUM,
6 LOW). All addressed in this branch.

HIGH

- H1: malformed `.devrail.yml` no longer silently treated as
  "no plugins declared". Both resolver and verifier now wrap yq parse
  via a `parse_plugin_count` helper that distinguishes "yq parse failure"
  (exit 2 with structured `config could not be parsed by yq` error event)
  from "no plugins entry" (silent success).

MEDIUM

- M1: lockfile verifier passes the .devrail.yml source URL via
  `strenv()` instead of string-interpolating it into the yq query.
  Defends against malicious/malformed source values breaking the query
  expression and failing open.
- M2: slug collisions detected upfront. Two plugin sources with
  matching `basename` (e.g. github.com/foo/credo and gitlab.com/bar/credo)
  would both map to the same `<plugins-dir>/credo/<rev>/` cache path,
  silently overwriting each other. Resolver now tracks SLUG_TO_SOURCE
  during the loop and emits a structured `plugin slug collision` error
  on the second occurrence.
- M3: fetch_to_cache now performs an atomic swap. Was
  `rm -rf target; mv fetch_dir target` (race window where target is
  absent). Now `mv target target.old; mv fetch_dir target; rm target.old`
  with rollback on the install-new mv. Concurrent readers see either
  the old tree or the new — never absent or half-populated.
- M4: `compute_content_hash` and `derive_slug` extracted to
  lib/plugin-cache.sh. Was duplicated between resolver and verifier;
  future drift risk eliminated.
- M5: resolver invokes plugin-validator.sh on the fetched manifest
  before computing content_hash. Authors hit manifest violations at
  `make plugins-update` time, not at every subsequent `make check`.

LOW

- L1: fetch_to_cache comment now reflects 4 args (not 3).
- L2: `derive_slug` strips `.git` suffix so https://example.com/foo.git
  yields cache path `<plugins-dir>/foo/<rev>/`.
- L3: lockfile entries written with double-quoted YAML scalars so
  source URLs containing colons, brackets, or yaml-reserved chars
  don't break parsing.
- L4: smoke test now covers `_plugins-update` no-op when no plugins
  declared (companion to existing `_plugins-verify` no-plugins case).
- L5: idempotent-fetch test now asserts `.devrail.sha` sentinel mtime
  is stable across re-runs, proving no re-clone happened (was just
  asserting the "plugin already cached" event).
- L6: smoke test exercises a `.git`-suffixed source URL.

Tests:

- tests/test-plugin-resolver.sh — 16/16 pass (was 11). New cases:
  malformed YAML, slug collision, .git suffix, no-plugins update no-op,
  mtime-based idempotency.
- tests/test-plugin-loader.sh — 11/11 pass (regression-safe).
- tests/smoke-rails.sh — 4/4 pass (regression-safe).
- make _check on dev-toolchain itself — pass.

Implementation note: cache files are written as root inside docker
(mktemp -d defaults to 0700). New tests that read inside plugins-cache
use a sidecar `docker run` (with `:ro` mount) instead of host-side
file checks, since the host user can't traverse 0700 directories.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@matthew-on-git matthew-on-git force-pushed the fix/13-3-review-followups branch from 52506fd to 6b479ca Compare May 3, 2026 13:49
@matthew-on-git matthew-on-git merged commit d92a982 into main May 3, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@matthew-on-git