From 2faf6bd888adb79358a6e1f186acae0b16fb5433 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:19:47 -0400 Subject: [PATCH 01/16] docs(plan): #771 lockfile dep-tracking design Extends resolveDependencies to call updateLockfileWithChecksum after each dep install (was: only parent tracked). Also drops the name@version gate so plain 'install ' captures resolved registry version. Closes #771 ACs. --- docs/plans/2026-05-24-lockfile-deps-design.md | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 docs/plans/2026-05-24-lockfile-deps-design.md diff --git a/docs/plans/2026-05-24-lockfile-deps-design.md b/docs/plans/2026-05-24-lockfile-deps-design.md new file mode 100644 index 00000000..1782b55f --- /dev/null +++ b/docs/plans/2026-05-24-lockfile-deps-design.md @@ -0,0 +1,89 @@ +# wfctl plugin install — lockfile dep tracking Design + +**Issue:** [workflow#771](https://github.com/GoCodeAlone/workflow/issues/771) +**Status:** Draft 2026-05-24 — awaiting adversarial design review +**Author:** Jon Langevin + +## Problem + +`wfctl plugin install @` recursively resolves + installs transitive `manifest.Dependencies` (`cmd/wfctl/plugin_deps.go:201 resolveDependencies`) but only the **parent** plugin gets a `.wfctl-lock.yaml` entry. Transitively-installed deps are written to disk via `installPluginFromManifest` (`plugin_deps.go:268`) but never get a lockfile entry. + +Second gap: plain `wfctl plugin install ` (no `@version`) skips lockfile update entirely (`plugin_install.go:256` `if _, ver := parseNameVersion(nameArg); ver != ""` gate). The resolved registry version isn't captured. + +## Solution + +Two small edits, single PR: + +### 1. Track deps in lockfile (`plugin_deps.go`) + +After successful dep install in `resolveDependencies` (after `plugin_deps.go:270` `resolved[dep.Name] = depManifest.Version`), compute the dep's binary SHA256 and call `updateLockfileWithChecksum` with the same args shape used for parent installs: + +```go +depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name) +depChecksum := "" +if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil { + depChecksum = cs +} else { + fmt.Fprintf(os.Stderr, "warning: could not hash dep binary %s: %v (lockfile will have no checksum)\n", depBinaryPath, hashErr) +} +updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum) +``` + +Matches the parent's pattern at `plugin_install.go:258-265`. Registry field is empty (matches parent behavior when installed via registry resolution). + +### 2. Drop the `name@version` gate (`plugin_install.go`) + +Remove the `if _, ver := parseNameVersion(nameArg); ver != ""` gate at line 256. Always update the lockfile with the resolved `manifest.Version` after a successful install. Per AC #2: plain `install ` should capture the resolved registry version, not skip the lockfile. + +## Files + +- `cmd/wfctl/plugin_deps.go:270` — append the dep-checksum + updateLockfileWithChecksum block. +- `cmd/wfctl/plugin_install.go:255-266` — drop the `ver != ""` gate; always update lockfile. +- `cmd/wfctl/plugin_install_lockfile_test.go` — add tests for transitive dep tracking + no-version install tracking. +- `cmd/wfctl/plugin_deps_test.go` — extend existing dep tests to assert lockfile entries. + +## Architecture choices + +| Choice | Picked | Rejected (reason) | +|---|---|---| +| Where to call updateLockfileWithChecksum for deps | inside resolveDependencies after each install | wrap installPluginFromManifest with always-lockfile (broader blast radius; some callers may not want auto-lock) | +| no-version install lockfile behavior | always update with resolved version | gate behind `--lock` flag (extra UX surface; AC says default) | +| Registry field on dep lockfile entry | empty string (matches parent's `sourceName` when via registry) | populate "registry" sentinel (no upstream consumer reads this field meaningfully today) | + +## Assumptions + +1. **All deps install via `installPluginFromManifest` writing binary at `//`** — verified per `plugin_install.go:259` parent pattern + `plugin_deps.go:268` dep install call (same function). Binary location consistent. +2. **`updateLockfileWithChecksum` is safe to call N times in a single install run** — verified per `plugin_lockfile.go:146`: idempotent merge into `lf.Plugins` map; later writes overwrite earlier entries for the same key. Multiple distinct dep names produce distinct entries. +3. **Failed dep install short-circuits before lockfile call** — verified per `plugin_deps.go:268-269`: `if err := installPluginFromManifest(...); err != nil { return ... }`. Lockfile entry only written on success. +4. **`hashFileSHA256` is the canonical hash function and returns empty + error on missing binary** — verified per existing `plugin_install.go:261` usage; "no checksum" fallback path documented. +5. **Lockfile schema field `Registry` accepts empty string** — verified per `plugin_lockfile.go:142-165` struct + Save serialization; YAML omits empty strings naturally per omitempty tags (or stays empty without breaking parse). + +## Failure modes + +- **Dep binary missing after install (race/disk error)**: `hashFileSHA256` returns error; warning printed; lockfile entry has empty checksum. Parent has same behavior. Acceptable. +- **Lockfile write fails (permission/disk)**: `updateLockfileWithChecksum` silently no-ops per existing semantics; install still succeeds. Existing behavior; not regressed. +- **Lockfile write race with concurrent wfctl invocations**: existing lockfile has no locking; new code inherits same race window. Out of scope for this PR (cross-cutting concern). +- **Plain `install ` against a registry whose latest moves between resolution and re-run**: lockfile captures the moment-in-time resolved version. Next run with `.wfctl-lock.yaml` present uses the locked version (already-installed-skip path at `plugin_deps.go:230-234` short-circuits). + +## Rollback + +Runtime-affecting (changes which entries `.wfctl-lock.yaml` accumulates). + +- **PR revert**: existing lockfiles with dep entries continue to parse (additive entries; no schema change). Future installs revert to parent-only tracking. Users who relied on dep entries lose them on next install. +- **Lockfile entries with empty checksum**: rare edge (binary hash failed); existing parent code already handles this case; no new failure path. +- **Backwards-compat**: `.wfctl-lock.yaml` schema unchanged (using existing PluginLockEntry struct). Older wfctl reading newer lockfile with dep entries: each entry is structurally valid; older wfctl just sees more entries than it would have written itself. No parse failure. + +## Top 3 doubts (self-challenge) + +1. **Plain `install ` always updating lockfile** changes user-visible behavior. Some operators may have deliberately avoided lockfile entries by omitting `@version`. Mitigation: the change matches the explicit AC; if users want opt-out, they can `--no-lock` (future flag, out of scope). +2. **Lockfile write happens after dep install but BEFORE parent install** — if parent install fails, lockfile has dep entries without parent. On retry the dep entries help (skip-already-installed). On manual diagnosis, the half-lockfile is informative. Existing behavior for plain installs: parent succeeds → lockfile written. New behavior: dep written before parent. If parent fails → dep stays in lockfile. Acceptable — install is best-effort transactional already (no atomic rollback). +3. **No dep version constraint persistence**: lockfile entries are version-pinned, but `dep.MinVersion`/`MaxVersion` constraints from the parent manifest are NOT captured in the dep's lockfile entry. Future re-install reads the lockfile-pinned version and skips constraint re-check (already-installed-skip path). Acceptable for AC scope; future tightening could record constraint metadata. + +## Non-goals + +- `wfctl plugin remove` lockfile cleanup (separate concern; existing behavior). +- Lockfile validation against installed state (covered by other tooling). +- Lockfile schema migration (additive only — no field changes). +- Constraint-metadata persistence on dep entries (deferred to future tightening). +- `--no-lock` opt-out flag (YAGNI for this PR). +- Concurrent-wfctl lockfile race handling (cross-cutting; pre-existing). From 7c1370053338f710b607f5f0222826a89e077355 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:23:55 -0400 Subject: [PATCH 02/16] docs(plan): #771 design cycle 2 (fix dual-format + no-clobber contract) Cycle 1 FAIL: 2 Critical (C1 early-return no-op on new-format dominant case; C2 dropped gate breaks installFromLockfile no-clobber contract). Cycle 2: - Refactor updateLockfileWithChecksum to fan out to BOTH legacy + new-format paths; preserve Platforms data on new-format merge. - Replace gate with installSkipLockfileUpdate package-level flag set+deferred by installFromLockfile (preserves no-clobber). - Document Repository field gap; defer fallback construction. - Test plan splits legacy + new-format scenarios; regression test for installFromLockfile invariant. --- docs/plans/2026-05-24-lockfile-deps-design.md | 169 ++++++++++++++---- 1 file changed, 134 insertions(+), 35 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps-design.md b/docs/plans/2026-05-24-lockfile-deps-design.md index 1782b55f..334dde7b 100644 --- a/docs/plans/2026-05-24-lockfile-deps-design.md +++ b/docs/plans/2026-05-24-lockfile-deps-design.md @@ -1,22 +1,83 @@ # wfctl plugin install — lockfile dep tracking Design **Issue:** [workflow#771](https://github.com/GoCodeAlone/workflow/issues/771) -**Status:** Draft 2026-05-24 — awaiting adversarial design review +**Status:** Revised cycle 2 2026-05-24 — awaiting re-review **Author:** Jon Langevin +## Revision history + +- **Cycle 1**: drafted minimal "call updateLockfileWithChecksum from resolveDependencies + drop @version gate". FAILED — 2 Critical: + - C1: new-format lockfile early-return at `plugin_lockfile.go:147` makes dep-write a no-op on the dominant case (any project with `version: 1` lockfile). + - C2: dropping the `@version` gate breaks `installFromLockfile`'s no-clobber contract (relied on by lockfile-driven install per `plugin_lockfile.go:115-118` comment). +- **Cycle 2** (this version): both formats covered. New `--no-lockfile-update` flag preserves `installFromLockfile`'s contract. Repository field gap acknowledged. Test plan splits legacy + new-format scenarios. + ## Problem -`wfctl plugin install @` recursively resolves + installs transitive `manifest.Dependencies` (`cmd/wfctl/plugin_deps.go:201 resolveDependencies`) but only the **parent** plugin gets a `.wfctl-lock.yaml` entry. Transitively-installed deps are written to disk via `installPluginFromManifest` (`plugin_deps.go:268`) but never get a lockfile entry. +`wfctl plugin install @` recursively resolves + installs transitive `manifest.Dependencies` (`cmd/wfctl/plugin_deps.go:201 resolveDependencies`) but only the **parent** plugin gets tracked, and only in some cases: -Second gap: plain `wfctl plugin install ` (no `@version`) skips lockfile update entirely (`plugin_install.go:256` `if _, ver := parseNameVersion(nameArg); ver != ""` gate). The resolved registry version isn't captured. +1. `updateLockfileWithChecksum` only touches the LEGACY `.wfctl-lock.yaml` format (`plugin_lockfile.go:142`). On projects with NEW-format lockfiles (`Version: 1`), the early-return at line 147 silently skips writes entirely → no parent tracking either. +2. Even on legacy format, the `if _, ver := parseNameVersion(nameArg); ver != ""` gate at `plugin_install.go:256` skips lockfile writes for plain `install ` invocations. +3. Recursive dep installs (`plugin_deps.go:268`) never call any lockfile update — neither format gets dep entries. ## Solution -Two small edits, single PR: +Three pieces, single PR: -### 1. Track deps in lockfile (`plugin_deps.go`) +### 1. New-format lockfile dep+parent merge (`config/wfctl_lockfile.go` consumer) -After successful dep install in `resolveDependencies` (after `plugin_deps.go:270` `resolved[dep.Name] = depManifest.Version`), compute the dep's binary SHA256 and call `updateLockfileWithChecksum` with the same args shape used for parent installs: +Add `mergeIntoNewFormatLockfile(name, version, source string)` helper in `cmd/wfctl/plugin_lockfile.go` that: + +```go +func mergeIntoNewFormatLockfile(name, version, source string) { + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil || lf == nil || lf.Version == 0 { + return // no new-format lockfile present; legacy path handles it + } + if lf.Plugins == nil { + lf.Plugins = make(map[string]config.WfctlLockPluginEntry) + } + existing := lf.Plugins[name] + // Preserve Platforms / Compatibility from existing entry — only update Version + Source. + existing.Version = version + if source != "" { + existing.Source = source + } + lf.Plugins[name] = existing + _ = config.SaveWfctlLockfile(wfctlLockPath, lf) +} +``` + +**Important**: do NOT clobber `existing.Platforms` (per-arch URLs + checksums) — those came from prior `wfctl plugin lock` generation. Merge-update preserves the heavy data, only refreshes Version + Source. + +### 2. Refactor `updateLockfileWithChecksum` to update BOTH formats + +Replace the early-return at `plugin_lockfile.go:147` with a fan-out: + +```go +func updateLockfileWithChecksum(pluginName, version, repository, registry, sha256Hash string) { + // New-format lockfile (version: 1) — merge entry preserving Platforms. + mergeIntoNewFormatLockfile(pluginName, version, repository) + + // Legacy-format .wfctl.yaml plugins block — existing path. + lf, err := loadPluginLockfile(wfctlLockPath) + if err != nil { + return + } + if lf.Plugins == nil { + lf.Plugins = make(map[string]PluginLockEntry) + } + lf.Plugins[pluginName] = PluginLockEntry{ + Version: version, Repository: repository, Registry: registry, SHA256: sha256Hash, + } + _ = lf.Save(wfctlLockPath) +} +``` + +The legacy path still runs (idempotent on new-format files which don't have `plugins:` in the legacy shape). Both writes are best-effort silent (preserves existing failure-tolerance posture). + +### 3. Track deps in `resolveDependencies` (`plugin_deps.go`) + +After successful dep install (after `plugin_deps.go:270` `resolved[dep.Name] = depManifest.Version`): ```go depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name) @@ -29,61 +90,99 @@ if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil { updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum) ``` -Matches the parent's pattern at `plugin_install.go:258-265`. Registry field is empty (matches parent behavior when installed via registry resolution). +### 4. Replace `@version` gate with explicit skip flag (preserves `installFromLockfile` contract) + +Per cycle-1 C2: `installFromLockfile` (`plugin_lockfile.go:115-118`) deliberately passes `name` without `@version` to avoid clobbering pinned entries before checksum verification. Removing the gate naively breaks that contract. + +Add package-level guard set/cleared by `installFromLockfile`: + +```go +// In plugin_install.go (package-level): +// installSkipLockfileUpdate suppresses lockfile updates during installFromLockfile's +// pre-verification install. Set by installFromLockfile; cleared in deferred reset. +var installSkipLockfileUpdate bool + +// In runPluginInstall, replace the gate at line 256: +if !installSkipLockfileUpdate { + binaryChecksum := "" + binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName) + if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + binaryChecksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) + } + updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum) +} +``` + +In `installFromLockfile`: +```go +installSkipLockfileUpdate = true +defer func() { installSkipLockfileUpdate = false }() +// ... existing call to runPluginInstall(...) +``` -### 2. Drop the `name@version` gate (`plugin_install.go`) +(Package-level boolean is acceptable — installs are sequential within a single wfctl invocation; no concurrent goroutines call this path.) -Remove the `if _, ver := parseNameVersion(nameArg); ver != ""` gate at line 256. Always update the lockfile with the resolved `manifest.Version` after a successful install. Per AC #2: plain `install ` should capture the resolved registry version, not skip the lockfile. +Plain `install ` (no `@version`) now writes to lockfile by default per AC #2. ## Files -- `cmd/wfctl/plugin_deps.go:270` — append the dep-checksum + updateLockfileWithChecksum block. -- `cmd/wfctl/plugin_install.go:255-266` — drop the `ver != ""` gate; always update lockfile. -- `cmd/wfctl/plugin_install_lockfile_test.go` — add tests for transitive dep tracking + no-version install tracking. -- `cmd/wfctl/plugin_deps_test.go` — extend existing dep tests to assert lockfile entries. +- `cmd/wfctl/plugin_lockfile.go` — refactor `updateLockfileWithChecksum` to fan out to both formats; add `mergeIntoNewFormatLockfile` helper. +- `cmd/wfctl/plugin_install.go:255-266` — replace `@version` gate with `!installSkipLockfileUpdate` guard; declare package-level bool. +- `cmd/wfctl/plugin_lockfile.go` (legacy `installFromLockfile`) — set+defer-clear `installSkipLockfileUpdate` around the inner `runPluginInstall` call. +- `cmd/wfctl/plugin_deps.go:270` — append dep checksum + updateLockfileWithChecksum. +- `cmd/wfctl/plugin_install_lockfile_test.go` — add tests for: (a) parent+dep tracking on LEGACY format, (b) parent+dep tracking on NEW format (verifying Platforms preserved), (c) no-version-install tracked on both formats, (d) `installFromLockfile` no-clobber invariant still holds (regression test). +- `cmd/wfctl/plugin_deps_test.go` — extend existing dep tests to assert lockfile entries on both formats. ## Architecture choices | Choice | Picked | Rejected (reason) | |---|---|---| -| Where to call updateLockfileWithChecksum for deps | inside resolveDependencies after each install | wrap installPluginFromManifest with always-lockfile (broader blast radius; some callers may not want auto-lock) | -| no-version install lockfile behavior | always update with resolved version | gate behind `--lock` flag (extra UX surface; AC says default) | -| Registry field on dep lockfile entry | empty string (matches parent's `sourceName` when via registry) | populate "registry" sentinel (no upstream consumer reads this field meaningfully today) | +| New-format coverage | refactor `updateLockfileWithChecksum` to fan out | document as out-of-scope (rejected: AC says "track in THE lockfile"; format split is invisible to operator) | +| `installFromLockfile` no-clobber preservation | package-level bool flag set+deferred | new `runPluginInstallNoLock` entrypoint (rejected: larger diff, more surface) | +| New-format dep merge semantics | preserve Platforms; update Version+Source only | overwrite entire entry (rejected: would clobber `wfctl plugin lock` generated per-arch data) | +| `Repository` field on dep entry | use `depManifest.Repository` as-is; empty if unset | fallback to constructed GitHub URL (rejected: per-org assumption + verify against existing registry data — defer to follow-up) | ## Assumptions -1. **All deps install via `installPluginFromManifest` writing binary at `//`** — verified per `plugin_install.go:259` parent pattern + `plugin_deps.go:268` dep install call (same function). Binary location consistent. -2. **`updateLockfileWithChecksum` is safe to call N times in a single install run** — verified per `plugin_lockfile.go:146`: idempotent merge into `lf.Plugins` map; later writes overwrite earlier entries for the same key. Multiple distinct dep names produce distinct entries. -3. **Failed dep install short-circuits before lockfile call** — verified per `plugin_deps.go:268-269`: `if err := installPluginFromManifest(...); err != nil { return ... }`. Lockfile entry only written on success. -4. **`hashFileSHA256` is the canonical hash function and returns empty + error on missing binary** — verified per existing `plugin_install.go:261` usage; "no checksum" fallback path documented. -5. **Lockfile schema field `Registry` accepts empty string** — verified per `plugin_lockfile.go:142-165` struct + Save serialization; YAML omits empty strings naturally per omitempty tags (or stays empty without breaking parse). +1. **`installPluginFromManifest` writes binary at `//`** for both parent and dep installs — verified per `plugin_install.go:259` (parent) and `plugin_deps.go:268` (dep, same function). Binary location consistent across both call sites. +2. **`config.LoadWfctlLockfile` returns `Version == 0` when file is missing OR legacy-format** — verified per `config/wfctl_lockfile.go:49-60`; missing file returns error, legacy parse returns zero-value struct. Helper's `Version > 0` check guards correctly. +3. **`config.SaveWfctlLockfile` accepts a lockfile struct with existing `Plugins` map and merges via replace** — verified per `config/wfctl_lockfile.go:62`; full re-serialization. Our merge logic pre-builds the desired final map shape. +4. **`installSkipLockfileUpdate` flag is safe as package-level state** — installs run sequentially within a single wfctl invocation; no goroutine concurrency across `runPluginInstall` calls. Cleared via defer; can't leak. +5. **`depManifest.Repository` is `omitempty`** but treated as best-effort (empty Source in new-format lockfile = entry still valid; legacy replay needs the field for source URL but lockfile is informational on the legacy path). +6. **Lockfile writes are idempotent** — `updateLockfileWithChecksum` called N times for N deps produces N distinct entries; existing parent code calls it once and works. New code inherits same semantics. ## Failure modes -- **Dep binary missing after install (race/disk error)**: `hashFileSHA256` returns error; warning printed; lockfile entry has empty checksum. Parent has same behavior. Acceptable. -- **Lockfile write fails (permission/disk)**: `updateLockfileWithChecksum` silently no-ops per existing semantics; install still succeeds. Existing behavior; not regressed. -- **Lockfile write race with concurrent wfctl invocations**: existing lockfile has no locking; new code inherits same race window. Out of scope for this PR (cross-cutting concern). -- **Plain `install ` against a registry whose latest moves between resolution and re-run**: lockfile captures the moment-in-time resolved version. Next run with `.wfctl-lock.yaml` present uses the locked version (already-installed-skip path at `plugin_deps.go:230-234` short-circuits). +- **Dep binary missing after install (race/disk)**: `hashFileSHA256` returns error → warning printed → lockfile entry has empty checksum. Matches parent behavior. Acceptable. +- **Lockfile write fails (permission/disk)**: silent no-op per existing semantics; install still succeeds. Not regressed. +- **New-format `Platforms` data dropped accidentally**: explicit `existing := lf.Plugins[name]` + selective field update preserves Platforms. Regression test required (Test plan §b). +- **`installFromLockfile` regression** (cycle-1 C2): mitigated by `installSkipLockfileUpdate` guard; regression test required (Test plan §d). +- **Concurrent wfctl invocations**: lockfile has no file-level locking; pre-existing race window. Not regressed; not addressed in this PR. +- **Plain `install ` against registry whose latest moves**: lockfile captures moment-in-time resolved version. Next run with lockfile present uses locked version (already-installed-skip path). ## Rollback -Runtime-affecting (changes which entries `.wfctl-lock.yaml` accumulates). +Runtime-affecting (changes lockfile-write behavior). -- **PR revert**: existing lockfiles with dep entries continue to parse (additive entries; no schema change). Future installs revert to parent-only tracking. Users who relied on dep entries lose them on next install. -- **Lockfile entries with empty checksum**: rare edge (binary hash failed); existing parent code already handles this case; no new failure path. -- **Backwards-compat**: `.wfctl-lock.yaml` schema unchanged (using existing PluginLockEntry struct). Older wfctl reading newer lockfile with dep entries: each entry is structurally valid; older wfctl just sees more entries than it would have written itself. No parse failure. +- **PR revert**: dep entries stop being written. Existing entries continue to parse. Lockfile-driven installs continue to work (gate replacement is internal-only refactor). +- **`installSkipLockfileUpdate` is package-private**: revert removes the flag + restores the gate. No external API change. +- **Lockfile schema unchanged**: both legacy `PluginLockEntry` and new `WfctlLockPluginEntry` structures untouched. +- **Backwards-compat**: older wfctl reading newer-written lockfile: more entries than would have been written; structurally valid; no parse failure. ## Top 3 doubts (self-challenge) -1. **Plain `install ` always updating lockfile** changes user-visible behavior. Some operators may have deliberately avoided lockfile entries by omitting `@version`. Mitigation: the change matches the explicit AC; if users want opt-out, they can `--no-lock` (future flag, out of scope). -2. **Lockfile write happens after dep install but BEFORE parent install** — if parent install fails, lockfile has dep entries without parent. On retry the dep entries help (skip-already-installed). On manual diagnosis, the half-lockfile is informative. Existing behavior for plain installs: parent succeeds → lockfile written. New behavior: dep written before parent. If parent fails → dep stays in lockfile. Acceptable — install is best-effort transactional already (no atomic rollback). -3. **No dep version constraint persistence**: lockfile entries are version-pinned, but `dep.MinVersion`/`MaxVersion` constraints from the parent manifest are NOT captured in the dep's lockfile entry. Future re-install reads the lockfile-pinned version and skips constraint re-check (already-installed-skip path). Acceptable for AC scope; future tightening could record constraint metadata. +1. **Dual-format write doubles I/O on every install.** Acceptable per design (best-effort + cheap YAML write); the operator-perceived value of unified dep tracking outweighs the cost. +2. **Package-level `installSkipLockfileUpdate` is global state.** Cleaner pattern is a context-passed flag, but requires threading through `runPluginInstall` signature. Defer to a future cleanup if pattern proliferates. Acceptable for this PR (single call site). +3. **`Platforms` preservation in new-format merge** could surprise — operator who manually crafted a new-format lockfile expects `wfctl plugin install` to be inert. But: design only merges when an entry is being newly written for an install we just performed, NOT for unrelated plugins. The merge is additive per-plugin. ## Non-goals -- `wfctl plugin remove` lockfile cleanup (separate concern; existing behavior). -- Lockfile validation against installed state (covered by other tooling). +- `wfctl plugin remove` lockfile cleanup (separate concern). - Lockfile schema migration (additive only — no field changes). - Constraint-metadata persistence on dep entries (deferred to future tightening). -- `--no-lock` opt-out flag (YAGNI for this PR). +- `--no-lock` user-facing opt-out flag (YAGNI for this PR; AC says default-on). - Concurrent-wfctl lockfile race handling (cross-cutting; pre-existing). +- `Repository` fallback URL construction for empty manifest entries (defer to follow-up; gap documented). +- Platforms data backfill for newly-tracked deps (deps only get Version+Source; per-arch archive metadata stays absent until next `wfctl plugin lock` run). From d9000f6af355e400a8c2e64f4cc2fe9d368eaed2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:27:00 -0400 Subject: [PATCH 03/16] docs(plan): #771 design cycle 3 (C3 installFromWfctlLockfile + I4 installPluginReqDirect) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cycle 2 FAIL: 1 Critical + 1 Important. Cycle 3: - C3: installSkipLockfileUpdate guard around installFromWfctlLockfile's runPluginInstall fallback call (mirror of cycle-2 fix for legacy installFromLockfile). - I4: append updateLockfileWithChecksum to installPluginReqDirect's parent install (closes user-intent asymmetry: --from-config deps were tracked, parents weren't). - Test plan §e + §f added for both regression coverage. --- docs/plans/2026-05-24-lockfile-deps-design.md | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps-design.md b/docs/plans/2026-05-24-lockfile-deps-design.md index 334dde7b..4dc57ad0 100644 --- a/docs/plans/2026-05-24-lockfile-deps-design.md +++ b/docs/plans/2026-05-24-lockfile-deps-design.md @@ -9,7 +9,8 @@ - **Cycle 1**: drafted minimal "call updateLockfileWithChecksum from resolveDependencies + drop @version gate". FAILED — 2 Critical: - C1: new-format lockfile early-return at `plugin_lockfile.go:147` makes dep-write a no-op on the dominant case (any project with `version: 1` lockfile). - C2: dropping the `@version` gate breaks `installFromLockfile`'s no-clobber contract (relied on by lockfile-driven install per `plugin_lockfile.go:115-118` comment). -- **Cycle 2** (this version): both formats covered. New `--no-lockfile-update` flag preserves `installFromLockfile`'s contract. Repository field gap acknowledged. Test plan splits legacy + new-format scenarios. +- **Cycle 2**: both formats covered + installSkipLockfileUpdate flag for installFromLockfile contract preservation. FAILED — 1 Critical (C3: installFromWfctlLockfile line-105 calls runPluginInstall ALSO unguarded; its in-memory `lf.Save` at line 116 silently clobbers the fan-out's writes) + 1 Important (I4: installPluginReqDirect skips parent lockfile track via direct installPluginFromManifest call, bypassing runPluginInstall). +- **Cycle 3** (this version): adds installSkipLockfileUpdate guard around installFromWfctlLockfile's runPluginInstall call site (mirror of cycle-2 fix for the legacy installFromLockfile path); adds updateLockfileWithChecksum call to installPluginReqDirect parent path; updates test plan with §e for installFromWfctlLockfile regression. ## Problem @@ -115,24 +116,47 @@ if !installSkipLockfileUpdate { } ``` -In `installFromLockfile`: +In `installFromLockfile` AND `installFromWfctlLockfile` (per cycle-3 C3 — both outer-frame installers hold in-memory `lf` and assume nothing underneath touches the on-disk file): + ```go installSkipLockfileUpdate = true defer func() { installSkipLockfileUpdate = false }() // ... existing call to runPluginInstall(...) ``` +Specifically: +- `cmd/wfctl/plugin_lockfile.go` line ~115 (legacy `installFromLockfile`). +- `cmd/wfctl/plugin_install_wfctllock.go` line ~99 (new-format `installFromWfctlLockfile` fallback path that calls `runPluginInstall(spec)` when per-platform archive is unavailable). + (Package-level boolean is acceptable — installs are sequential within a single wfctl invocation; no concurrent goroutines call this path.) Plain `install ` (no `@version`) now writes to lockfile by default per AC #2. +### 5. Cover `installPluginReqDirect` parent (cycle-3 I4) + +`cmd/wfctl/plugin_deps.go:82-112` (`installPluginReqDirect`) is the `--from-config` / `installFromWorkflowConfig` parent path. It calls `installPluginFromManifest` directly (NOT `runPluginInstall`) so the §4 guard logic doesn't apply. Add lockfile tracking explicitly after the parent install succeeds: + +```go +// In installPluginReqDirect, after successful installPluginFromManifest: +binaryPath := filepath.Join(pluginDir, req.Name, req.Name) +checksum := "" +if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + checksum = cs +} +updateLockfileWithChecksum(req.Name, manifest.Version, manifest.Repository, "", checksum) +``` + +Pre-existing gap (not regressed by this PR) but trivially closeable in the same PR — same chokepoint helper. Closes the user-intent asymmetry where `--from-config` deps land in lockfile but parents don't. + ## Files - `cmd/wfctl/plugin_lockfile.go` — refactor `updateLockfileWithChecksum` to fan out to both formats; add `mergeIntoNewFormatLockfile` helper. - `cmd/wfctl/plugin_install.go:255-266` — replace `@version` gate with `!installSkipLockfileUpdate` guard; declare package-level bool. - `cmd/wfctl/plugin_lockfile.go` (legacy `installFromLockfile`) — set+defer-clear `installSkipLockfileUpdate` around the inner `runPluginInstall` call. +- `cmd/wfctl/plugin_install_wfctllock.go` (new-format `installFromWfctlLockfile`) — set+defer-clear `installSkipLockfileUpdate` around the inner `runPluginInstall` fallback call (cycle-3 C3). +- `cmd/wfctl/plugin_deps.go:82-112` (`installPluginReqDirect` parent path) — append `updateLockfileWithChecksum` after success (cycle-3 I4). - `cmd/wfctl/plugin_deps.go:270` — append dep checksum + updateLockfileWithChecksum. -- `cmd/wfctl/plugin_install_lockfile_test.go` — add tests for: (a) parent+dep tracking on LEGACY format, (b) parent+dep tracking on NEW format (verifying Platforms preserved), (c) no-version-install tracked on both formats, (d) `installFromLockfile` no-clobber invariant still holds (regression test). +- `cmd/wfctl/plugin_install_lockfile_test.go` — add tests for: (a) parent+dep tracking on LEGACY format, (b) parent+dep tracking on NEW format (verifying Platforms preserved), (c) no-version-install tracked on both formats, (d) `installFromLockfile` no-clobber invariant still holds (regression test), (e) `installFromWfctlLockfile` no-clobber invariant when fallback fires (regression test for cycle-3 C3), (f) `installPluginReqDirect` parent gets tracked (cycle-3 I4 coverage). - `cmd/wfctl/plugin_deps_test.go` — extend existing dep tests to assert lockfile entries on both formats. ## Architecture choices From f489f838a1a16475c7752b31bcea3bd57f6751be Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:29:10 -0400 Subject: [PATCH 04/16] docs(plan): #771 design cycle 4 (extend guard to per-arch installFromURL call site) --- docs/plans/2026-05-24-lockfile-deps-design.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/plans/2026-05-24-lockfile-deps-design.md b/docs/plans/2026-05-24-lockfile-deps-design.md index 4dc57ad0..6d3fada0 100644 --- a/docs/plans/2026-05-24-lockfile-deps-design.md +++ b/docs/plans/2026-05-24-lockfile-deps-design.md @@ -10,7 +10,8 @@ - C1: new-format lockfile early-return at `plugin_lockfile.go:147` makes dep-write a no-op on the dominant case (any project with `version: 1` lockfile). - C2: dropping the `@version` gate breaks `installFromLockfile`'s no-clobber contract (relied on by lockfile-driven install per `plugin_lockfile.go:115-118` comment). - **Cycle 2**: both formats covered + installSkipLockfileUpdate flag for installFromLockfile contract preservation. FAILED — 1 Critical (C3: installFromWfctlLockfile line-105 calls runPluginInstall ALSO unguarded; its in-memory `lf.Save` at line 116 silently clobbers the fan-out's writes) + 1 Important (I4: installPluginReqDirect skips parent lockfile track via direct installPluginFromManifest call, bypassing runPluginInstall). -- **Cycle 3** (this version): adds installSkipLockfileUpdate guard around installFromWfctlLockfile's runPluginInstall call site (mirror of cycle-2 fix for the legacy installFromLockfile path); adds updateLockfileWithChecksum call to installPluginReqDirect parent path; updates test plan with §e for installFromWfctlLockfile regression. +- **Cycle 3**: adds installSkipLockfileUpdate guard around installFromWfctlLockfile's runPluginInstall fallback call + updateLockfileWithChecksum in installPluginReqDirect parent path. FAILED — 1 Important (I5: per-arch installFromURL call site in installFromWfctlLockfile line 86 also needs guard; under §2 fan-out it triggers merge-then-clobber by outer in-memory lf.Save). +- **Cycle 4** (this version): extends §4 guard to also wrap the per-arch installFromURL call site at line 86. Symmetric coverage of all outer-in-memory-lf-holding sites. ## Problem @@ -127,6 +128,7 @@ defer func() { installSkipLockfileUpdate = false }() Specifically: - `cmd/wfctl/plugin_lockfile.go` line ~115 (legacy `installFromLockfile`). - `cmd/wfctl/plugin_install_wfctllock.go` line ~99 (new-format `installFromWfctlLockfile` fallback path that calls `runPluginInstall(spec)` when per-platform archive is unavailable). +- `cmd/wfctl/plugin_install_wfctllock.go` line ~86 (new-format `installFromWfctlLockfile` per-platform archive path that calls `installFromURL` — which under §2 fan-out triggers `mergeIntoNewFormatLockfile` and would be re-clobbered by the outer in-memory `lf.Save` at line 116). Same symmetric guard. (Cycle-4 I5.) (Package-level boolean is acceptable — installs are sequential within a single wfctl invocation; no concurrent goroutines call this path.) From f52a2a2c2ce2ea1a9ca6533ac429a6b019783384 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:32:23 -0400 Subject: [PATCH 05/16] docs(plan): #771 design cycle 5 (move guard inside updateLockfileWithChecksum chokepoint) Cycle 4 FAIL: 1 Critical (gate at runPluginInstall:256 inert for line-86 installFromURL path which writes lockfile at 846). Cycle 5 adopts reviewer Option 1: gate INSIDE updateLockfileWithChecksum. Single chokepoint; all 5 call sites (256/846/934/dep-recursion/installPluginReqDirect) auto-respect. Resolves C1 + I6 together. --- docs/plans/2026-05-24-lockfile-deps-design.md | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps-design.md b/docs/plans/2026-05-24-lockfile-deps-design.md index 6d3fada0..f6ff45ab 100644 --- a/docs/plans/2026-05-24-lockfile-deps-design.md +++ b/docs/plans/2026-05-24-lockfile-deps-design.md @@ -11,7 +11,8 @@ - C2: dropping the `@version` gate breaks `installFromLockfile`'s no-clobber contract (relied on by lockfile-driven install per `plugin_lockfile.go:115-118` comment). - **Cycle 2**: both formats covered + installSkipLockfileUpdate flag for installFromLockfile contract preservation. FAILED — 1 Critical (C3: installFromWfctlLockfile line-105 calls runPluginInstall ALSO unguarded; its in-memory `lf.Save` at line 116 silently clobbers the fan-out's writes) + 1 Important (I4: installPluginReqDirect skips parent lockfile track via direct installPluginFromManifest call, bypassing runPluginInstall). - **Cycle 3**: adds installSkipLockfileUpdate guard around installFromWfctlLockfile's runPluginInstall fallback call + updateLockfileWithChecksum in installPluginReqDirect parent path. FAILED — 1 Important (I5: per-arch installFromURL call site in installFromWfctlLockfile line 86 also needs guard; under §2 fan-out it triggers merge-then-clobber by outer in-memory lf.Save). -- **Cycle 4** (this version): extends §4 guard to also wrap the per-arch installFromURL call site at line 86. Symmetric coverage of all outer-in-memory-lf-holding sites. +- **Cycle 4**: extends §4 guard to per-arch installFromURL call site at line 86. FAILED — 1 Critical (C1: gate at runPluginInstall:256 is mechanically inert; line-86 path bypasses runPluginInstall and reaches updateLockfileWithChecksum at plugin_install.go:846 directly, never consulting the flag) + 1 Important (I6: §3 dep tracking unconditionally writes during outer guarded installs). +- **Cycle 5** (this version): adopts reviewer Option 1 — move installSkipLockfileUpdate check INSIDE updateLockfileWithChecksum itself. Single chokepoint; every call site (256, 846, 934, dep recursion, installPluginReqDirect) auto-respects the guard. C1 and I6 dissolve together. ## Problem @@ -96,27 +97,27 @@ updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository Per cycle-1 C2: `installFromLockfile` (`plugin_lockfile.go:115-118`) deliberately passes `name` without `@version` to avoid clobbering pinned entries before checksum verification. Removing the gate naively breaks that contract. -Add package-level guard set/cleared by `installFromLockfile`: +Add package-level guard at the CHOKEPOINT (cycle-5 fix per C1: gate at `runPluginInstall:256` is mechanically inert because `installFromURL` reaches `updateLockfileWithChecksum` at line 846 without going through `runPluginInstall`; same for `installFromLocal` at 934. Move the check INSIDE the helper so every call site is implicitly covered): ```go // In plugin_install.go (package-level): -// installSkipLockfileUpdate suppresses lockfile updates during installFromLockfile's -// pre-verification install. Set by installFromLockfile; cleared in deferred reset. +// installSkipLockfileUpdate suppresses ALL lockfile writes when set. Used by +// outer installers (installFromLockfile / installFromWfctlLockfile) that hold +// the lockfile in memory and re-save it themselves; without this guard inner +// install paths would mutate the on-disk file under them. var installSkipLockfileUpdate bool -// In runPluginInstall, replace the gate at line 256: -if !installSkipLockfileUpdate { - binaryChecksum := "" - binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName) - if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { - binaryChecksum = cs - } else { - fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) +// In plugin_lockfile.go, at the TOP of updateLockfileWithChecksum: +func updateLockfileWithChecksum(pluginName, version, repository, registry, sha256Hash string) { + if installSkipLockfileUpdate { + return } - updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum) + // ... rest of existing function body (fan-out per §2) } ``` +Drop the `name@version` gate at `plugin_install.go:256`. The new line-265 `updateLockfileWithChecksum` call always fires; the helper itself decides whether to skip. + In `installFromLockfile` AND `installFromWfctlLockfile` (per cycle-3 C3 — both outer-frame installers hold in-memory `lf` and assume nothing underneath touches the on-disk file): ```go From fe61378669e667fa9ad11b574d711de7f9770a63 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:36:11 -0400 Subject: [PATCH 06/16] docs(plan): #771 implementation plan 6 tasks, 1 PR. Chokepoint guard at top of updateLockfileWithChecksum + fan-out to both legacy + new-format lockfile + outer-frame guards in installFromLockfile/installFromWfctlLockfile + dep tracking in resolveDependencies + parent tracking in installPluginReqDirect. --- docs/plans/2026-05-24-lockfile-deps.md | 593 +++++++++++++++++++++++++ 1 file changed, 593 insertions(+) create mode 100644 docs/plans/2026-05-24-lockfile-deps.md diff --git a/docs/plans/2026-05-24-lockfile-deps.md b/docs/plans/2026-05-24-lockfile-deps.md new file mode 100644 index 00000000..b6ae17d8 --- /dev/null +++ b/docs/plans/2026-05-24-lockfile-deps.md @@ -0,0 +1,593 @@ +# lockfile dep tracking Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Extend `wfctl plugin install` to write transitively-installed deps to `.wfctl-lock.yaml` (both legacy + new-format) AND always-track plain `install ` (drop `@version` gate), while preserving `installFromLockfile` / `installFromWfctlLockfile` no-clobber contracts via a chokepoint guard inside `updateLockfileWithChecksum`. + +**Architecture:** Single PR. Add `mergeIntoNewFormatLockfile` helper + fan-out write inside refactored `updateLockfileWithChecksum`. Add package-level `installSkipLockfileUpdate` flag checked at TOP of `updateLockfileWithChecksum` (single chokepoint). Set+defer-clear the flag in both outer-frame installers (`installFromLockfile`, `installFromWfctlLockfile`) around their inner `runPluginInstall`/`installFromURL` calls. Drop the `name@version` gate. Extend `resolveDependencies` and `installPluginReqDirect` to call the helper. + +**Tech Stack:** Go (wfctl), `config.WfctlLockfile` / `config.SaveWfctlLockfile` for new-format, existing `loadPluginLockfile` for legacy. + +**Base branch:** `main` + +**Design doc:** `docs/plans/2026-05-24-lockfile-deps-design.md` (cycle 5 PASS adversarial). + +**Issue:** workflow#771 + +--- + +## Scope Manifest + +**PR Count:** 1 +**Tasks:** 6 +**Estimated Lines of Change:** ~250 + +**Out of scope:** +- `wfctl plugin remove` lockfile cleanup +- `Repository` fallback URL construction for empty manifest entries +- Platforms-data backfill for newly-tracked deps +- `--no-lock` user-facing opt-out flag +- Concurrent-wfctl lockfile race handling (pre-existing) +- Constraint-metadata persistence on dep entries +- Lockfile schema migration (additive only) + +**PR Grouping:** + +| PR # | Title | Tasks | Branch | +|------|-------|-------|--------| +| 1 | feat(wfctl): lockfile dep tracking + always-track plain installs (workflow#771) | Task 1, Task 2, Task 3, Task 4, Task 5, Task 6 | feat/771-lockfile-deps | + +**Status:** Draft + +--- + +### Task 1: Add `installSkipLockfileUpdate` chokepoint guard + `mergeIntoNewFormatLockfile` helper + +**Change class:** Internal logic refactor. + +**Files:** +- Modify: `cmd/wfctl/plugin_install.go` — add package-level `var installSkipLockfileUpdate bool`. +- Modify: `cmd/wfctl/plugin_lockfile.go` — add `mergeIntoNewFormatLockfile` + insert chokepoint guard at top of `updateLockfileWithChecksum`; remove existing `LoadWfctlLockfile().Version > 0` early-return; fan-out to both formats. +- Test: `cmd/wfctl/plugin_install_lockfile_test.go` — add tests for chokepoint guard + new-format write. + +**Step 1: Write the failing tests** + +In `cmd/wfctl/plugin_install_lockfile_test.go` (edit existing SINGLE import block; do NOT add a second `import (...)`): + +```go +func TestUpdateLockfileWithChecksum_GuardSkips(t *testing.T) { + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + t.Fatal(err) + } + installSkipLockfileUpdate = true + defer func() { installSkipLockfileUpdate = false }() + updateLockfileWithChecksum("foo", "1.0.0", "", "", "") + b, _ := os.ReadFile(wfctlLockPath) + if strings.Contains(string(b), "foo") { + t.Errorf("guard should have suppressed write; got: %s", b) + } +} + +func TestUpdateLockfileWithChecksum_NewFormatFanOut(t *testing.T) { + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins:\n bar:\n version: 0.1.0\n source: github.com/x/bar\n platforms:\n linux_amd64:\n url: https://example.com/bar\n sha256: deadbeef\n"), 0o600); err != nil { + t.Fatal(err) + } + updateLockfileWithChecksum("bar", "1.2.3", "github.com/x/bar-new", "", "feedface") + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatal(err) + } + got := lf.Plugins["bar"] + if got.Version != "1.2.3" { + t.Errorf("Version = %q, want 1.2.3", got.Version) + } + if got.Source != "github.com/x/bar-new" { + t.Errorf("Source = %q, want github.com/x/bar-new", got.Source) + } + if len(got.Platforms) == 0 { + t.Errorf("Platforms should be preserved; got empty") + } + if got.Platforms["linux_amd64"].URL != "https://example.com/bar" { + t.Errorf("Platforms URL clobbered: %v", got.Platforms) + } +} +``` + +**Step 2: Run tests — verify FAIL** + +Run: `GOWORK=off go test -run "TestUpdateLockfileWithChecksum_GuardSkips|TestUpdateLockfileWithChecksum_NewFormatFanOut" -count=1 ./cmd/wfctl/...` +Expected: FAIL — `installSkipLockfileUpdate undefined` AND new-format write doesn't happen. + +**Step 3: Implement** + +In `cmd/wfctl/plugin_install.go` (add after existing package-level vars near top of file): + +```go +// installSkipLockfileUpdate suppresses ALL lockfile writes when set. Outer +// installers (installFromLockfile / installFromWfctlLockfile) hold the +// lockfile in memory and re-save it themselves; without this guard, inner +// install paths' lockfile writes would be silently overwritten by the +// outer re-save (workflow#771 cycle-5 chokepoint pattern). +var installSkipLockfileUpdate bool +``` + +In `cmd/wfctl/plugin_lockfile.go`, add `mergeIntoNewFormatLockfile` helper: + +```go +// mergeIntoNewFormatLockfile updates the new-format .wfctl-lock.yaml's +// Plugins[name] entry, preserving Platforms / Compatibility data while +// refreshing Version + Source. No-ops if the lockfile is missing or +// legacy-format (Version == 0). +func mergeIntoNewFormatLockfile(name, version, source string) { + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil || lf == nil || lf.Version == 0 { + return + } + if lf.Plugins == nil { + lf.Plugins = make(map[string]config.WfctlLockPluginEntry) + } + existing := lf.Plugins[name] + existing.Version = version + if source != "" { + existing.Source = source + } + lf.Plugins[name] = existing + _ = config.SaveWfctlLockfile(wfctlLockPath, lf) +} +``` + +Then refactor `updateLockfileWithChecksum` in `cmd/wfctl/plugin_lockfile.go` (current line 146): remove the `if newLF, err := config.LoadWfctlLockfile(...); err == nil && newLF.Version > 0 { return }` early-return; replace with chokepoint guard + fan-out: + +```go +func updateLockfileWithChecksum(pluginName, version, repository, registry, sha256Hash string) { + if installSkipLockfileUpdate { + return + } + // New-format lockfile (version: 1) — merge entry preserving Platforms. + mergeIntoNewFormatLockfile(pluginName, version, repository) + + // Legacy-format .wfctl.yaml plugins block — existing path. + lf, err := loadPluginLockfile(wfctlLockPath) + if err != nil { + return + } + if lf.Plugins == nil { + lf.Plugins = make(map[string]PluginLockEntry) + } + lf.Plugins[pluginName] = PluginLockEntry{ + Version: version, + Repository: repository, + Registry: registry, + SHA256: sha256Hash, + } + _ = lf.Save(wfctlLockPath) +} +``` + +Add `"github.com/GoCodeAlone/workflow/config"` to the existing single import block in `plugin_lockfile.go` if not already present. + +**Step 4: Run tests — verify PASS** + +Run: `GOWORK=off go test -run "TestUpdateLockfileWithChecksum_GuardSkips|TestUpdateLockfileWithChecksum_NewFormatFanOut" -count=1 ./cmd/wfctl/...` +Expected: both PASS. + +**Step 5: Commit** + +```bash +git add cmd/wfctl/plugin_install.go cmd/wfctl/plugin_lockfile.go cmd/wfctl/plugin_install_lockfile_test.go +git commit -m "feat(wfctl): chokepoint guard + new-format fan-out in updateLockfileWithChecksum (workflow#771 Task 1)" +``` + +**Rollback:** revert this commit — `updateLockfileWithChecksum` returns to legacy-only behavior with prior early-return guard. No data migration. + +--- + +### Task 2: Drop `name@version` gate in `runPluginInstall` + +**Change class:** Internal logic refactor (gate removal). + +**Files:** +- Modify: `cmd/wfctl/plugin_install.go` lines 255-266 — remove the `if _, ver := parseNameVersion(nameArg); ver != ""` wrapper around lockfile update; always call the helper. +- Test: `cmd/wfctl/plugin_install_lockfile_test.go` — add test for plain-name install tracking. + +**Step 1: Write the failing test** + +Append: + +```go +func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + t.Fatal(err) + } + // Directly invoke updateLockfileWithChecksum with the resolved manifest + // version to simulate the post-gate-removal behavior the production code + // will produce: plain `install ` resolves a version then writes + // it unconditionally. + updateLockfileWithChecksum("baz", "2.0.0", "github.com/x/baz", "", "abc123") + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatal(err) + } + if lf.Plugins["baz"].Version != "2.0.0" { + t.Errorf("plain install should track; got %v", lf.Plugins["baz"]) + } +} +``` + +**Step 2: Run test — verify it passes** (existing helper from Task 1; this confirms the helper supports the "no @version" use case before we wire the call site). + +Run: `GOWORK=off go test -run TestRunPluginInstall_NoVersionTracksLockfile -count=1 ./cmd/wfctl/...` +Expected: PASS (validates Task 1's helper works for the gateless flow). + +**Step 3: Remove the gate** + +In `cmd/wfctl/plugin_install.go` lines 255-266, replace: + +```go +// Update .wfctl-lock.yaml lockfile if name@version was provided. +if _, ver := parseNameVersion(nameArg); ver != "" { + pluginName = normalizePluginName(pluginName) + binaryChecksum := "" + binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName) + if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + binaryChecksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) + } + updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum) +} +``` + +with: + +```go +// Update .wfctl-lock.yaml lockfile (workflow#771: always-track, gate removed). +// The chokepoint guard inside updateLockfileWithChecksum (Task 1) is responsible +// for suppressing writes during outer-frame installers. +pluginName = normalizePluginName(pluginName) +binaryChecksum := "" +binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName) +if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + binaryChecksum = cs +} else { + fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) +} +updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum) +``` + +**Step 4: Run all lockfile tests — verify PASS** + +Run: `GOWORK=off go test -run "TestUpdateLockfileWithChecksum|TestRunPluginInstall" -count=1 ./cmd/wfctl/...` +Expected: all PASS including the existing `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` (must still pass because the test's setup may rely on the gate; if it fails, the test invariant is now covered by the Task 3 guard instead). + +If `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` fails: confirm the test's setup mimics the `installFromLockfile`-driven flow (sets `installSkipLockfileUpdate` if exposed). If the test was relying on the gate's `ver != ""` check, the test itself needs an update — but that update belongs in Task 3 (where `installSkipLockfileUpdate` is set by outer-frame installers), not here. + +**Step 5: Commit** + +```bash +git add cmd/wfctl/plugin_install.go cmd/wfctl/plugin_install_lockfile_test.go +git commit -m "feat(wfctl): always-track plain install in lockfile (drop @version gate) (workflow#771 Task 2)" +``` + +**Rollback:** revert this commit — gate restored; plain installs no longer track. + +--- + +### Task 3: Set guard in outer-frame installers (`installFromLockfile` + `installFromWfctlLockfile`) + +**Change class:** Internal logic refactor (preserves no-clobber contracts). + +**Files:** +- Modify: `cmd/wfctl/plugin_lockfile.go` line ~115 (`installFromLockfile`) — set+defer-clear `installSkipLockfileUpdate` around the inner `runPluginInstall` call. +- Modify: `cmd/wfctl/plugin_install_wfctllock.go` — set+defer-clear `installSkipLockfileUpdate` around BOTH the per-arch `installFromURL` call (line ~86) AND the fallback `runPluginInstall` call (line ~99). +- Test: `cmd/wfctl/plugin_install_lockfile_test.go` — add regression tests for both no-clobber contracts. + +**Step 1: Write the failing tests** + +Append: + +```go +func TestInstallFromLockfile_NoClobberInvariant(t *testing.T) { + // Test that when installFromLockfile is active, inner runPluginInstall + // does NOT mutate the on-disk lockfile (would clobber pinned entries + // before checksum verification). + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins:\n pinned:\n version: 1.0.0\n source: github.com/x/pinned\n"), 0o600); err != nil { + t.Fatal(err) + } + installSkipLockfileUpdate = true + defer func() { installSkipLockfileUpdate = false }() + // Simulate inner install attempting to write a different version + updateLockfileWithChecksum("pinned", "9.9.9", "github.com/x/pinned-bad", "", "badchecksum") + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatal(err) + } + if lf.Plugins["pinned"].Version != "1.0.0" { + t.Errorf("inner install clobbered pinned entry; got %s, want 1.0.0", lf.Plugins["pinned"].Version) + } +} +``` + +**Step 2: Run test — verify PASS** (the test directly validates Task 1's guard; the actual outer-frame setter sites are wired in Step 3). + +Run: `GOWORK=off go test -run TestInstallFromLockfile_NoClobberInvariant -count=1 ./cmd/wfctl/...` +Expected: PASS (validates the guard mechanism works end-to-end). + +**Step 3: Wire the guard in outer-frame installers** + +In `cmd/wfctl/plugin_lockfile.go`, find `installFromLockfile` (around line 89). Before the `installArgs := []string{...}` block (around line 115-118), add: + +```go +installSkipLockfileUpdate = true +defer func() { installSkipLockfileUpdate = false }() +``` + +In `cmd/wfctl/plugin_install_wfctllock.go`, find `installFromWfctlLockfile` (around line 27). At the TOP of the per-plugin loop body (before line 86's `installFromURL` call and before the line-99 fallback `runPluginInstall` call), add: + +```go +installSkipLockfileUpdate = true +// Cleared at loop iteration end via defer; for safety, we use an inline +// reset on each iteration since defer-in-loop would only fire at function exit. +``` + +(Better: wrap the loop body in an anonymous function with `defer func() { installSkipLockfileUpdate = false }()`, OR explicitly clear at the bottom of the iteration. Pick whichever the implementer finds clearest; defer-in-anon-func is more idiomatic Go.) + +```go +for name, entry := range lf.Plugins { + func() { + installSkipLockfileUpdate = true + defer func() { installSkipLockfileUpdate = false }() + // ... existing per-plugin install logic (lines 56-119) + }() +} +``` + +**Step 4: Run all install-related tests — verify PASS** + +Run: `GOWORK=off go test -run "TestInstallFromLockfile|TestInstallFromWfctlLockfile|TestRunPluginInstall|TestUpdateLockfileWithChecksum" -count=1 -timeout 120s ./cmd/wfctl/...` +Expected: all PASS including the existing `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` (now covered by Task 3 guards instead of the dropped @version gate). + +**Step 5: Commit** + +```bash +git add cmd/wfctl/plugin_lockfile.go cmd/wfctl/plugin_install_wfctllock.go cmd/wfctl/plugin_install_lockfile_test.go +git commit -m "feat(wfctl): set installSkipLockfileUpdate in outer-frame installers (workflow#771 Task 3)" +``` + +**Rollback:** revert this commit — outer-frame guards removed; inner installs from lockfile resume risking on-disk clobber (regression to pre-Task-2 state but with Task 2's gate removed → real lockfile mutation possible). Task 3 MUST land with Task 2 for no-clobber contract preservation. + +--- + +### Task 4: Track transitive deps in `resolveDependencies` + +**Change class:** Internal logic refactor. + +**Files:** +- Modify: `cmd/wfctl/plugin_deps.go:268-273` — append hash + updateLockfileWithChecksum after successful dep install. +- Test: `cmd/wfctl/plugin_deps_test.go` — assert dep entries appear in lockfile post-install. + +**Step 1: Write the failing test** + +Append to `cmd/wfctl/plugin_deps_test.go` (edit existing single import block; do NOT add a second): + +```go +func TestResolveDependencies_TracksDepsInLockfile(t *testing.T) { + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + t.Fatal(err) + } + // Directly invoke updateLockfileWithChecksum to simulate dep install path. + // (Full resolveDependencies test infrastructure already exists; this verifies + // the chokepoint helper is reachable from the dep recursion site post-Task-4.) + updateLockfileWithChecksum("depA", "0.5.0", "github.com/x/depA", "", "depAsha") + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatal(err) + } + if lf.Plugins["depA"].Version != "0.5.0" { + t.Errorf("dep not tracked; got %v", lf.Plugins["depA"]) + } +} +``` + +**Step 2: Run test — verify PASS** (Task 1's helper supports dep-name writes). + +Run: `GOWORK=off go test -run TestResolveDependencies_TracksDepsInLockfile -count=1 ./cmd/wfctl/...` +Expected: PASS. + +**Step 3: Wire dep tracking in `resolveDependencies`** + +In `cmd/wfctl/plugin_deps.go` after the existing `resolved[dep.Name] = depManifest.Version` (around line 271), append: + +```go + // Track dep in lockfile (workflow#771 Task 4). The chokepoint guard + // inside updateLockfileWithChecksum (Task 1) suppresses writes when + // running under an outer-frame installer (installFromLockfile etc.). + depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name) + depChecksum := "" + if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil { + depChecksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash dep binary %s: %v (lockfile will have no checksum)\n", depBinaryPath, hashErr) + } + updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum) +``` + +Add `"path/filepath"` and `"os"` to the existing single import block in `plugin_deps.go` if not already present. + +**Step 4: Run dep tests — verify PASS** + +Run: `GOWORK=off go test -run "TestResolveDependencies" -count=1 -timeout 60s ./cmd/wfctl/...` +Expected: existing dep tests PASS + new lockfile assertion PASS. + +**Step 5: Commit** + +```bash +git add cmd/wfctl/plugin_deps.go cmd/wfctl/plugin_deps_test.go +git commit -m "feat(wfctl): track transitive deps in lockfile via resolveDependencies (workflow#771 Task 4)" +``` + +**Rollback:** revert this commit — deps stop being tracked. Parent + plain-install tracking from Tasks 1-3 unaffected. + +--- + +### Task 5: Track parent in `installPluginReqDirect` (`--from-config` path) + +**Change class:** Internal logic refactor. + +**Files:** +- Modify: `cmd/wfctl/plugin_deps.go` `installPluginReqDirect` function (around line 82-112) — append `updateLockfileWithChecksum` after successful install. +- Test: `cmd/wfctl/plugin_deps_test.go` — assert parent entry appears for `--from-config` install path. + +**Step 1: Write the failing test** + +Append: + +```go +func TestInstallPluginReqDirect_TracksParentInLockfile(t *testing.T) { + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + t.Fatal(err) + } + // Simulate the post-Task-5 write the production code will produce. + updateLockfileWithChecksum("from-config-parent", "1.5.0", "github.com/x/parent", "", "sha") + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatal(err) + } + if lf.Plugins["from-config-parent"].Version != "1.5.0" { + t.Errorf("from-config parent not tracked; got %v", lf.Plugins["from-config-parent"]) + } +} +``` + +**Step 2: Run test — verify PASS** (Task 1's helper). + +Run: `GOWORK=off go test -run TestInstallPluginReqDirect_TracksParentInLockfile -count=1 ./cmd/wfctl/...` +Expected: PASS. + +**Step 3: Wire the call in `installPluginReqDirect`** + +In `cmd/wfctl/plugin_deps.go` `installPluginReqDirect` function, AFTER the successful `installPluginFromManifest` call (around line 111, before the function `return nil`), append: + +```go + // Track parent in lockfile (workflow#771 Task 5). Closes the asymmetry + // where --from-config dep installs were tracked via Task 4 but parent + // installs via this path were not. + binaryPath := filepath.Join(pluginDir, req.Name, req.Name) + checksum := "" + if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + checksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) + } + updateLockfileWithChecksum(req.Name, manifest.Version, manifest.Repository, "", checksum) +``` + +(Verify variable names `pluginDir`, `req`, `manifest` against the actual function signature; substitute as needed.) + +**Step 4: Run tests — verify PASS** + +Run: `GOWORK=off go test -run "TestInstallPluginReqDirect" -count=1 -timeout 60s ./cmd/wfctl/...` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add cmd/wfctl/plugin_deps.go cmd/wfctl/plugin_deps_test.go +git commit -m "feat(wfctl): track --from-config parent in lockfile via installPluginReqDirect (workflow#771 Task 5)" +``` + +**Rollback:** revert this commit — `--from-config` parent tracking gap returns; deps via Task 4 still tracked. + +--- + +### Task 6: Final verification + regression sweep + +**Change class:** Internal logic refactor (test-only). + +**Files:** +- Test: `cmd/wfctl/plugin_install_lockfile_test.go` — extend coverage if any gap surfaces. + +**Step 1: Run full lockfile + install test suite** + +Run: `GOWORK=off go test -run "TestUpdateLockfileWithChecksum|TestRunPluginInstall|TestInstallFromLockfile|TestInstallFromWfctlLockfile|TestResolveDependencies|TestInstallPluginReqDirect" -count=1 -timeout 180s ./cmd/wfctl/...` +Expected: all PASS, including existing `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` (now covered by Task 3 guard). + +**Step 2: Run `go vet` + lint** + +Run: `GOWORK=off go vet ./...` and `GOWORK=off golangci-lint run ./cmd/wfctl/...` +Expected: no warnings. + +**Step 3: Run full wfctl test suite — regression check** + +Run: `GOWORK=off go test -count=1 -timeout 300s ./cmd/wfctl/...` +Expected: all PASS. + +**Step 4: Commit any gap-fill tests** + +If Step 1-3 surfaces a gap (e.g., a test that relied on the now-dropped @version gate semantics), add coverage + commit: + +```bash +git add cmd/wfctl/plugin_install_lockfile_test.go +git commit -m "test(wfctl): regression coverage for lockfile dep tracking (workflow#771 Task 6)" +``` + +If no gap surfaces, this task produces no commit (acknowledge in the Task-6 commit message footer of Task 5 or skip silently). + +**Rollback:** N/A (test-only). + +--- + +## Final verification (post-Task-6) + +Before opening the PR: + +```bash +# 1. All tests pass +GOWORK=off go test -count=1 -timeout 300s ./cmd/wfctl/... + +# 2. Lint clean +GOWORK=off go vet ./... +GOWORK=off golangci-lint run + +# 3. wfctl --help still works +GOWORK=off go build -o /tmp/wfctl ./cmd/wfctl && /tmp/wfctl plugin install --help + +# 4. End-to-end smoke (out-of-tree) +# Install a plugin with deps, verify lockfile has both entries: +# /tmp/wfctl plugin install workflow-plugin-something +# grep -E "^ (something|dep-of-something):" .wfctl-lock.yaml +``` + +## Rollback + +This PR touches lockfile-write behavior across multiple call sites. Rollback path: + +- `git revert ` reverts all 6 commits cleanly. Existing `.wfctl-lock.yaml` files with dep entries continue to parse (additive entries; no schema change). +- **Lockfile entries added by this PR persist after revert** — older wfctl ignores them harmlessly (encoding/json + yaml ignore unknown keys, and entries are structurally valid). +- **Outer-frame contract regression risk**: if Task 2 (gate drop) is reverted without also reverting Task 3 (outer-frame guards), `installFromLockfile` behavior is unchanged. If Task 3 is reverted without reverting Task 2, the gate is gone AND no outer-frame guard exists → inner installs clobber lockfile entries. **Revert Tasks 2 + 3 together if reverting either.** + +Backwards-compat: subcommand behavior expansion is additive (more entries written). Existing scripts that parse the lockfile see additional entries — structurally compatible. From 5f1265ab60f3c8dd789ec598ec0a84bd0b5a8123 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:41:59 -0400 Subject: [PATCH 07/16] docs(plan): #771 plan cycle 2 (override reviewer C1 + fix fake-TDD C2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cycle 1 FAIL: 2 Critical. Cycle 2: - C1 OVERRIDDEN: line 846 is in installFromURL (760+), NOT installPluginFromManifest (325-431). installPluginFromManifest verified to NOT call updateLockfileWithChecksum. Tasks 4+5 ARE necessary; reviewer misread. Override documented inline. - C2 ACCEPTED: each task's Step 1.5 added — rewrite tests to invoke actual production entrypoints via httptest pattern (precedent at plugin_install_lockfile_test.go:38+). - Task 2 Step 4 wrong commentary corrected (test survives via mergeIntoNewFormatLockfile's preservation, not Task 3 guards). - Task 3 Step 3 anon-func wrap explicitly picked. - Task 1 var comment: t.Parallel() warning. - Task 5 variable names pre-resolved. --- docs/plans/2026-05-24-lockfile-deps.md | 72 ++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 11 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps.md b/docs/plans/2026-05-24-lockfile-deps.md index b6ae17d8..fcb16705 100644 --- a/docs/plans/2026-05-24-lockfile-deps.md +++ b/docs/plans/2026-05-24-lockfile-deps.md @@ -39,6 +39,15 @@ **Status:** Draft +## Reviewer override (plan cycle 1 → cycle 2) + +Cycle-1 plan-phase adversarial reviewer flagged 2 Critical: + +- **C1 (Tasks 4+5 redundant due to line-846 chokepoint)**: OVERRIDDEN as factually incorrect. Line 846 is inside `installFromURL` (lines 760+), NOT `installPluginFromManifest` (lines 325-431). `installPluginFromManifest` body verified: ends at line 431 with `commitPluginStagingDir` + `Printf("Installed ...")` — NO `updateLockfileWithChecksum` call. `resolveDependencies:268` and `installPluginReqDirect:111` both call `installPluginFromManifest` directly (not via `installFromURL`), so neither triggers any lockfile write today. Tasks 4 + 5 ARE necessary. Override documented per reviewer's "Options" rubric. +- **C2 (fake-TDD: tests call helper directly, not changed entrypoint)**: ACCEPTED. Cycle 2 rewrites each task's Step-1 test to invoke the changed production entrypoint via httptest server pattern (precedent at `plugin_install_lockfile_test.go:568+`). + +Plus cycle-2 fixes Important: anon-func explicit in Task 3, parallel-test warning on global state, variable-name pre-resolution in Task 5, wrong Task-2 Step-4 commentary deleted. + --- ### Task 1: Add `installSkipLockfileUpdate` chokepoint guard + `mergeIntoNewFormatLockfile` helper @@ -116,6 +125,11 @@ In `cmd/wfctl/plugin_install.go` (add after existing package-level vars near top // lockfile in memory and re-save it themselves; without this guard, inner // install paths' lockfile writes would be silently overwritten by the // outer re-save (workflow#771 cycle-5 chokepoint pattern). +// +// NOTE: package-level state. Tests touching this MUST NOT call t.Parallel() — +// cross-test flag leakage would silently break lockfile invariants. See +// design doc §"Top 3 doubts #2" for rationale on rejecting context.Context +// threading. var installSkipLockfileUpdate bool ``` @@ -226,10 +240,36 @@ func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { } ``` -**Step 2: Run test — verify it passes** (existing helper from Task 1; this confirms the helper supports the "no @version" use case before we wire the call site). +**Step 1.5 (cycle-2 fix per reviewer C2): rewrite to invoke `runPluginInstall` via httptest pattern** + +The Step-1 test above directly invokes the helper which won't catch the gate's presence. Replace with an end-to-end test using the existing `httptest.NewServer` pattern (precedent at `plugin_install_lockfile_test.go:568+`): + +```go +func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { + dir := t.TempDir() + savedPath := wfctlLockPath + wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") + defer func() { wfctlLockPath = savedPath }() + // New-format empty lockfile so the chokepoint fan-out fires. + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } + + // Reuse the existing httptest pattern — see TestRunPluginInstall_LocksAfterInstall + // at line 38 of this file. Serve a minimal plugin tarball + manifest; call + // runPluginInstall with PLAIN name (no @version). Assert lockfile entry + // appears post-install. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // ... mirror existing test's manifest+tarball serving logic + })) + defer srv.Close() + // ... call runPluginInstall([]string{"--plugin-dir", dir, "--source", srv.URL, "baz"}) + // Then assert lf.Plugins["baz"].Version == manifest.Version +} +``` + +**Implementer note**: use existing `TestRunPluginInstall_LocksAfterInstall` (line 38) as the literal template — copy its httptest setup, change `baz@1.0.0` → bare `baz` in the install args, and remove the `@version` parse assertion. The gate's presence will cause the lockfile NOT to be written → test FAILs → drop gate → test PASSes. Run: `GOWORK=off go test -run TestRunPluginInstall_NoVersionTracksLockfile -count=1 ./cmd/wfctl/...` -Expected: PASS (validates Task 1's helper works for the gateless flow). +Expected: FAIL with "plugins.baz not in lockfile" UNTIL Step 3's gate removal. **Step 3: Remove the gate** @@ -272,7 +312,7 @@ updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, so Run: `GOWORK=off go test -run "TestUpdateLockfileWithChecksum|TestRunPluginInstall" -count=1 ./cmd/wfctl/...` Expected: all PASS including the existing `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` (must still pass because the test's setup may rely on the gate; if it fails, the test invariant is now covered by the Task 3 guard instead). -If `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` fails: confirm the test's setup mimics the `installFromLockfile`-driven flow (sets `installSkipLockfileUpdate` if exposed). If the test was relying on the gate's `ver != ""` check, the test itself needs an update — but that update belongs in Task 3 (where `installSkipLockfileUpdate` is set by outer-frame installers), not here. +**Cycle-2 correction** (per reviewer C3 misread): the existing `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile` will continue to PASS even without Task 3's guards, NOT because of guards but because `mergeIntoNewFormatLockfile` (Task 1) preserves `Platforms` and never touches top-level `SHA256` field. The test's assertions about `entry.SHA256 == ""` and `platform.URL == "https://example.test/original.tar.gz"` and `platform.SHA256 == "archive-sha-from-lock"` ALL survive the fan-out because the helper is intentionally non-clobbering. No test update needed in this task. **Step 5: Commit** @@ -324,10 +364,14 @@ func TestInstallFromLockfile_NoClobberInvariant(t *testing.T) { } ``` -**Step 2: Run test — verify PASS** (the test directly validates Task 1's guard; the actual outer-frame setter sites are wired in Step 3). +**Step 1.5 (cycle-2 fix per reviewer C2): rewrite to invoke `installFromLockfile` via httptest** + +The Step-1 test sets the guard manually without exercising production wiring. Replace with full-flow test that invokes `installFromLockfile` against a legacy-format `.wfctl.yaml` with a `plugins:` block; ensure inner install attempts run but the on-disk pinned entry is preserved (regression catches missing/misplaced guard at outer-frame). + +Reuse precedent at `plugin_install_lockfile_test.go:38+` for httptest setup; substitute `runPluginInstall` invocation with `installFromLockfile(...)` and assert the on-disk `lf.Plugins["pinned"].Version` matches the ORIGINAL pin, not the post-install version. Run: `GOWORK=off go test -run TestInstallFromLockfile_NoClobberInvariant -count=1 ./cmd/wfctl/...` -Expected: PASS (validates the guard mechanism works end-to-end). +Expected: FAIL (without Step-3 guard, inner install would clobber pinned entry). **Step 3: Wire the guard in outer-frame installers** @@ -346,7 +390,7 @@ installSkipLockfileUpdate = true // reset on each iteration since defer-in-loop would only fire at function exit. ``` -(Better: wrap the loop body in an anonymous function with `defer func() { installSkipLockfileUpdate = false }()`, OR explicitly clear at the bottom of the iteration. Pick whichever the implementer finds clearest; defer-in-anon-func is more idiomatic Go.) +**Cycle-2 decision per reviewer Important**: use the anon-func wrapper (idiomatic Go, defer-safe even if inner panics). The inline-reset alternative is rejected because it leaks state on panic. Final form: ```go for name, entry := range lf.Plugins { @@ -358,6 +402,8 @@ for name, entry := range lf.Plugins { } ``` +Move ALL existing loop-body code inside the anonymous function. Both line-86 `installFromURL` and line-99 fallback `runPluginInstall` calls land inside the guarded scope. + **Step 4: Run all install-related tests — verify PASS** Run: `GOWORK=off go test -run "TestInstallFromLockfile|TestInstallFromWfctlLockfile|TestRunPluginInstall|TestUpdateLockfileWithChecksum" -count=1 -timeout 120s ./cmd/wfctl/...` @@ -409,10 +455,12 @@ func TestResolveDependencies_TracksDepsInLockfile(t *testing.T) { } ``` -**Step 2: Run test — verify PASS** (Task 1's helper supports dep-name writes). +**Step 1.5 (cycle-2 fix per reviewer C2): rewrite to invoke `resolveDependencies` end-to-end** + +Replace direct-helper-call with a real `resolveDependencies` invocation using the existing `plugin_deps_test.go:170` pattern (which already wires `manifest`, `pluginDir`, `cfgFile`, `resolved` map). Add post-call assertion that `lf.Plugins[].Version` is set. Run: `GOWORK=off go test -run TestResolveDependencies_TracksDepsInLockfile -count=1 ./cmd/wfctl/...` -Expected: PASS. +Expected: FAIL — dep tracking line is not yet added at Step 3. After Step 3's append, expected PASS. **Step 3: Wire dep tracking in `resolveDependencies`** @@ -483,10 +531,12 @@ func TestInstallPluginReqDirect_TracksParentInLockfile(t *testing.T) { } ``` -**Step 2: Run test — verify PASS** (Task 1's helper). +**Step 1.5 (cycle-2 fix per reviewer C2): rewrite to invoke `installPluginReqDirect` end-to-end** + +Use the existing `installPluginReqDirect` test pattern (precedent: similar tests at plugin_deps_test.go) to invoke the function directly with a `config.PluginRequirement`. Assert lockfile entry appears post-call. Run: `GOWORK=off go test -run TestInstallPluginReqDirect_TracksParentInLockfile -count=1 ./cmd/wfctl/...` -Expected: PASS. +Expected: FAIL — `installPluginReqDirect` doesn't write lockfile yet; PASS after Step 3. **Step 3: Wire the call in `installPluginReqDirect`** @@ -506,7 +556,7 @@ In `cmd/wfctl/plugin_deps.go` `installPluginReqDirect` function, AFTER the succe updateLockfileWithChecksum(req.Name, manifest.Version, manifest.Repository, "", checksum) ``` -(Verify variable names `pluginDir`, `req`, `manifest` against the actual function signature; substitute as needed.) +**Cycle-2 pre-resolved**: verified against `cmd/wfctl/plugin_deps.go:82`: `func installPluginReqDirect(pluginDir, registryCfgPath string, req config.PluginRequirement) error` — `pluginDir` and `req` are parameters; `manifest` is the local var at line 95 of the function. Use as written above. **Step 4: Run tests — verify PASS** From 85df616057511b8e0efa0335893c52242dc90f66 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:48:00 -0400 Subject: [PATCH 08/16] docs(plan): #771 plan cycle 3 (fix const + anon-func compile errors + normalized name) Cycle 2 FAIL: 2 Critical (test snippets use const assignment; anon-func wrapper breaks continue) + 4 Important. Cycle 3: - CYC2-C1: tests use os.Chdir(dir) pattern (precedent at existing tests:30/95/160) instead of reassigning const wfctlLockPath. - CYC2-C2: Task 3 installFromWfctlLockfile uses FUNCTION-SCOPE guard (set+defer at top of function), not anon-func-per-iter (illegal continue inside func literal). - CYC2-I1: Task 5 uses normalized pluginName (line 87) for both hash path AND lockfile key, not raw req.Name. - CYC2-I3: test name reference fixed to existing TestRunPluginInstall_DoesNotRewriteNewFormatLockfile. --- docs/plans/2026-05-24-lockfile-deps.md | 118 +++++++++++++++---------- 1 file changed, 72 insertions(+), 46 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps.md b/docs/plans/2026-05-24-lockfile-deps.md index fcb16705..7b25b86f 100644 --- a/docs/plans/2026-05-24-lockfile-deps.md +++ b/docs/plans/2026-05-24-lockfile-deps.md @@ -66,10 +66,13 @@ In `cmd/wfctl/plugin_install_lockfile_test.go` (edit existing SINGLE import bloc ```go func TestUpdateLockfileWithChecksum_GuardSkips(t *testing.T) { dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() - if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) + if err := os.WriteFile(".wfctl-lock.yaml", []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } installSkipLockfileUpdate = true @@ -83,14 +86,17 @@ func TestUpdateLockfileWithChecksum_GuardSkips(t *testing.T) { func TestUpdateLockfileWithChecksum_NewFormatFanOut(t *testing.T) { dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins:\n bar:\n version: 0.1.0\n source: github.com/x/bar\n platforms:\n linux_amd64:\n url: https://example.com/bar\n sha256: deadbeef\n"), 0o600); err != nil { t.Fatal(err) } updateLockfileWithChecksum("bar", "1.2.3", "github.com/x/bar-new", "", "feedface") - lf, err := config.LoadWfctlLockfile(wfctlLockPath) + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") if err != nil { t.Fatal(err) } @@ -141,7 +147,7 @@ In `cmd/wfctl/plugin_lockfile.go`, add `mergeIntoNewFormatLockfile` helper: // refreshing Version + Source. No-ops if the lockfile is missing or // legacy-format (Version == 0). func mergeIntoNewFormatLockfile(name, version, source string) { - lf, err := config.LoadWfctlLockfile(wfctlLockPath) + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") if err != nil || lf == nil || lf.Version == 0 { return } @@ -219,10 +225,13 @@ Append: ```go func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() - if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) + if err := os.WriteFile(".wfctl-lock.yaml", []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } // Directly invoke updateLockfileWithChecksum with the resolved manifest @@ -230,7 +239,7 @@ func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { // will produce: plain `install ` resolves a version then writes // it unconditionally. updateLockfileWithChecksum("baz", "2.0.0", "github.com/x/baz", "", "abc123") - lf, err := config.LoadWfctlLockfile(wfctlLockPath) + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") if err != nil { t.Fatal(err) } @@ -247,13 +256,16 @@ The Step-1 test above directly invokes the helper which won't catch the gate's p ```go func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) // New-format empty lockfile so the chokepoint fan-out fires. - if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } + if err := os.WriteFile(".wfctl-lock.yaml", []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } - // Reuse the existing httptest pattern — see TestRunPluginInstall_LocksAfterInstall + // Reuse the existing httptest pattern — see TestRunPluginInstall_DoesNotRewriteNewFormatLockfile (the only end-to-end httptest precedent in this file at line 548) // at line 38 of this file. Serve a minimal plugin tarball + manifest; call // runPluginInstall with PLAIN name (no @version). Assert lockfile entry // appears post-install. @@ -266,7 +278,7 @@ func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { } ``` -**Implementer note**: use existing `TestRunPluginInstall_LocksAfterInstall` (line 38) as the literal template — copy its httptest setup, change `baz@1.0.0` → bare `baz` in the install args, and remove the `@version` parse assertion. The gate's presence will cause the lockfile NOT to be written → test FAILs → drop gate → test PASSes. +**Implementer note**: use existing `TestRunPluginInstall_DoesNotRewriteNewFormatLockfile (the only end-to-end httptest precedent in this file at line 548)` (line 38) as the literal template — copy its httptest setup, change `baz@1.0.0` → bare `baz` in the install args, and remove the `@version` parse assertion. The gate's presence will cause the lockfile NOT to be written → test FAILs → drop gate → test PASSes. Run: `GOWORK=off go test -run TestRunPluginInstall_NoVersionTracksLockfile -count=1 ./cmd/wfctl/...` Expected: FAIL with "plugins.baz not in lockfile" UNTIL Step 3's gate removal. @@ -344,9 +356,12 @@ func TestInstallFromLockfile_NoClobberInvariant(t *testing.T) { // does NOT mutate the on-disk lockfile (would clobber pinned entries // before checksum verification). dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins:\n pinned:\n version: 1.0.0\n source: github.com/x/pinned\n"), 0o600); err != nil { t.Fatal(err) } @@ -354,7 +369,7 @@ func TestInstallFromLockfile_NoClobberInvariant(t *testing.T) { defer func() { installSkipLockfileUpdate = false }() // Simulate inner install attempting to write a different version updateLockfileWithChecksum("pinned", "9.9.9", "github.com/x/pinned-bad", "", "badchecksum") - lf, err := config.LoadWfctlLockfile(wfctlLockPath) + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") if err != nil { t.Fatal(err) } @@ -390,19 +405,16 @@ installSkipLockfileUpdate = true // reset on each iteration since defer-in-loop would only fire at function exit. ``` -**Cycle-2 decision per reviewer Important**: use the anon-func wrapper (idiomatic Go, defer-safe even if inner panics). The inline-reset alternative is rejected because it leaks state on panic. Final form: +**Cycle-3 fix per reviewer CYC2-C2**: anon-func-per-iter wrapper is illegal because `installFromWfctlLockfile`'s loop body has 6× `continue` statements (lines 62, 68, 79, 89, 94, 108 of `plugin_install_wfctllock.go`) — `continue` inside an anonymous function literal is a compile error ("continue is not in a loop"). Use function-scope guard instead: ```go -for name, entry := range lf.Plugins { - func() { - installSkipLockfileUpdate = true - defer func() { installSkipLockfileUpdate = false }() - // ... existing per-plugin install logic (lines 56-119) - }() -} +// At the TOP of installFromWfctlLockfile (line ~28), BEFORE the for-loop: +installSkipLockfileUpdate = true +defer func() { installSkipLockfileUpdate = false }() +// ... rest of function (loop with continues intact) ``` -Move ALL existing loop-body code inside the anonymous function. Both line-86 `installFromURL` and line-99 fallback `runPluginInstall` calls land inside the guarded scope. +This protects the entire function call (including ALL per-plugin per-arch and per-plugin fallback runPluginInstall calls). Single set+defer pair at function scope; no per-iteration churn. Acceptable because `installFromWfctlLockfile` is itself an outer-frame installer — nothing nested calls it recursively, so the flag stays correctly scoped for the full lifetime of the function. **Step 4: Run all install-related tests — verify PASS** @@ -435,17 +447,20 @@ Append to `cmd/wfctl/plugin_deps_test.go` (edit existing single import block; do ```go func TestResolveDependencies_TracksDepsInLockfile(t *testing.T) { dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() - if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) + if err := os.WriteFile(".wfctl-lock.yaml", []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } // Directly invoke updateLockfileWithChecksum to simulate dep install path. // (Full resolveDependencies test infrastructure already exists; this verifies // the chokepoint helper is reachable from the dep recursion site post-Task-4.) updateLockfileWithChecksum("depA", "0.5.0", "github.com/x/depA", "", "depAsha") - lf, err := config.LoadWfctlLockfile(wfctlLockPath) + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") if err != nil { t.Fatal(err) } @@ -513,15 +528,18 @@ Append: ```go func TestInstallPluginReqDirect_TracksParentInLockfile(t *testing.T) { dir := t.TempDir() - savedPath := wfctlLockPath - wfctlLockPath = filepath.Join(dir, ".wfctl-lock.yaml") - defer func() { wfctlLockPath = savedPath }() - if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + // wfctlLockPath is a const (cmd/wfctl/plugin_lockfile.go:20); existing tests + // redirect via os.Chdir + relative-path semantics (precedent at + // plugin_install_lockfile_test.go:30, :95, :160, etc). + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) + if err := os.WriteFile(".wfctl-lock.yaml", []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { t.Fatal(err) } // Simulate the post-Task-5 write the production code will produce. updateLockfileWithChecksum("from-config-parent", "1.5.0", "github.com/x/parent", "", "sha") - lf, err := config.LoadWfctlLockfile(wfctlLockPath) + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") if err != nil { t.Fatal(err) } @@ -546,17 +564,25 @@ In `cmd/wfctl/plugin_deps.go` `installPluginReqDirect` function, AFTER the succe // Track parent in lockfile (workflow#771 Task 5). Closes the asymmetry // where --from-config dep installs were tracked via Task 4 but parent // installs via this path were not. - binaryPath := filepath.Join(pluginDir, req.Name, req.Name) + // Cycle-3 fix per reviewer CYC2-I1: use the function's NORMALIZED pluginName + // (already computed at line 87 via normalizePluginName(rawName)), NOT raw + // req.Name. installPluginFromManifest installs at pluginDir//; + // raw req.Name "workflow-plugin-auth" would hash-miss at pluginDir/workflow-plugin-auth/... + // and produce an asymmetric lockfile key vs runPluginInstall's writes. + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) checksum := "" if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { checksum = cs } else { fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) } - updateLockfileWithChecksum(req.Name, manifest.Version, manifest.Repository, "", checksum) + updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, "", checksum) ``` -**Cycle-2 pre-resolved**: verified against `cmd/wfctl/plugin_deps.go:82`: `func installPluginReqDirect(pluginDir, registryCfgPath string, req config.PluginRequirement) error` — `pluginDir` and `req` are parameters; `manifest` is the local var at line 95 of the function. Use as written above. +**Cycle-3 pre-resolved**: verified against `cmd/wfctl/plugin_deps.go:82-95`: +- Signature: `func installPluginReqDirect(pluginDir, registryCfgPath string, req config.PluginRequirement) error`. +- Line 87: `pluginName := normalizePluginName(rawName)` — use THIS for both the hash path AND the lockfile key (not raw `req.Name`). +- `manifest` is the local var at line 95. **Step 4: Run tests — verify PASS** From a1d94b0dd3405ab1b79f7021caa11f3299d924e5 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:53:19 -0400 Subject: [PATCH 09/16] docs(plan): #771 plan cycle 4 (mutually-exclusive formats + key normalization) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cycle 3 FAIL: 2 Critical (CYC3-C1: legacy Save wipes Platforms via PluginLockEntry lacking field; CYC3-C2: helper doesn't normalize keys → duplicate short/long-form entries). Cycle 4: - Mutually-exclusive format paths: if v1 lockfile present, write ONLY v1 (returns true; skip legacy). Legacy write only when v1 absent. Restores Platforms protection that the dropped early-return provided. - mergeIntoNewFormatLockfile returns bool + scans for normalized-equivalent existing key when exact match missing (handles real-world long-form lockfile keys). - Task 4 dep tracking normalizes depKey for both hash path AND lockfile key (CYC3-I1). - Task 3 installFromLockfile guard moved to function scope (CYC3-I2; mirrors installFromWfctlLockfile fix). --- docs/plans/2026-05-24-lockfile-deps.md | 55 +++++++++++++++++++------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps.md b/docs/plans/2026-05-24-lockfile-deps.md index 7b25b86f..8cbee51f 100644 --- a/docs/plans/2026-05-24-lockfile-deps.md +++ b/docs/plans/2026-05-24-lockfile-deps.md @@ -141,40 +141,59 @@ var installSkipLockfileUpdate bool In `cmd/wfctl/plugin_lockfile.go`, add `mergeIntoNewFormatLockfile` helper: +In `cmd/wfctl/plugin_lockfile.go`, add `mergeIntoNewFormatLockfile` helper. Cycle-4 fix per reviewer CYC3-C2: helper handles BOTH key forms (real-world lockfiles key entries by long-form `workflow-plugin-auth` while runPluginInstall normalizes to `auth`). Returns bool so caller knows whether v1 path fired: + ```go // mergeIntoNewFormatLockfile updates the new-format .wfctl-lock.yaml's // Plugins[name] entry, preserving Platforms / Compatibility data while -// refreshing Version + Source. No-ops if the lockfile is missing or -// legacy-format (Version == 0). -func mergeIntoNewFormatLockfile(name, version, source string) { - lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") +// refreshing Version + Source. Returns true iff lockfile is v1 format +// (so caller can skip the legacy write path). +// +// name passed in normalized form. Helper handles existing entries keyed +// by long-form (e.g. "workflow-plugin-auth") by scanning for any key +// matching normalizePluginName. +func mergeIntoNewFormatLockfile(name, version, source string) bool { + lf, err := config.LoadWfctlLockfile(wfctlLockPath) if err != nil || lf == nil || lf.Version == 0 { - return + return false } if lf.Plugins == nil { lf.Plugins = make(map[string]config.WfctlLockPluginEntry) } - existing := lf.Plugins[name] + // Lookup: exact match first, else scan for normalized-equivalent key. + key := name + if _, ok := lf.Plugins[name]; !ok { + for existingKey := range lf.Plugins { + if normalizePluginName(existingKey) == name { + key = existingKey + break + } + } + } + existing := lf.Plugins[key] existing.Version = version if source != "" { existing.Source = source } - lf.Plugins[name] = existing + lf.Plugins[key] = existing _ = config.SaveWfctlLockfile(wfctlLockPath, lf) + return true } ``` -Then refactor `updateLockfileWithChecksum` in `cmd/wfctl/plugin_lockfile.go` (current line 146): remove the `if newLF, err := config.LoadWfctlLockfile(...); err == nil && newLF.Version > 0 { return }` early-return; replace with chokepoint guard + fan-out: +Then refactor `updateLockfileWithChecksum` with chokepoint guard + MUTUALLY-EXCLUSIVE format paths (cycle-4 fix per CYC3-C1: `PluginLockEntry` has no `Platforms` field, so the legacy `Save()` re-marshals over the v1 file's plugins block destroying Platforms; protect by returning if v1 helper handled it): ```go func updateLockfileWithChecksum(pluginName, version, repository, registry, sha256Hash string) { if installSkipLockfileUpdate { return } - // New-format lockfile (version: 1) — merge entry preserving Platforms. - mergeIntoNewFormatLockfile(pluginName, version, repository) - - // Legacy-format .wfctl.yaml plugins block — existing path. + // V1 format takes precedence: if .wfctl-lock.yaml exists with version: 1, + // write ONLY to that path (preserves Platforms). Skip legacy save entirely. + if mergeIntoNewFormatLockfile(pluginName, version, repository) { + return + } + // Legacy fallback: no v1 lockfile present; write to legacy .wfctl.yaml plugins block. lf, err := loadPluginLockfile(wfctlLockPath) if err != nil { return @@ -390,9 +409,11 @@ Expected: FAIL (without Step-3 guard, inner install would clobber pinned entry). **Step 3: Wire the guard in outer-frame installers** -In `cmd/wfctl/plugin_lockfile.go`, find `installFromLockfile` (around line 89). Before the `installArgs := []string{...}` block (around line 115-118), add: +In `cmd/wfctl/plugin_lockfile.go`, find `installFromLockfile` (around line 89). Cycle-4 fix per CYC3-I2: place guard at FUNCTION SCOPE (top of function, before the for-loop opens at line 106), NOT inside the loop body — mirrors the same fix applied to `installFromWfctlLockfile`. Single set+defer pair: ```go +// At the TOP of installFromLockfile (after the v1 branch's early-return at line 92, +// before the legacy loop opens at line 106): installSkipLockfileUpdate = true defer func() { installSkipLockfileUpdate = false }() ``` @@ -485,14 +506,18 @@ In `cmd/wfctl/plugin_deps.go` after the existing `resolved[dep.Name] = depManife // Track dep in lockfile (workflow#771 Task 4). The chokepoint guard // inside updateLockfileWithChecksum (Task 1) suppresses writes when // running under an outer-frame installer (installFromLockfile etc.). - depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name) + // Cycle-4 fix per reviewer CYC3-I1: normalize dep key for both hash + // path AND lockfile key — keeps dep entries consistent with parent + // entries (which use normalizePluginName per runPluginInstall line 257). + depKey := normalizePluginName(dep.Name) + depBinaryPath := filepath.Join(pluginDir, depKey, depKey) depChecksum := "" if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil { depChecksum = cs } else { fmt.Fprintf(os.Stderr, "warning: could not hash dep binary %s: %v (lockfile will have no checksum)\n", depBinaryPath, hashErr) } - updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum) + updateLockfileWithChecksum(depKey, depManifest.Version, depManifest.Repository, "", depChecksum) ``` Add `"path/filepath"` and `"os"` to the existing single import block in `plugin_deps.go` if not already present. From 721004c17a542ea3d2b73f3f2a64d4dfa74c4d6f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:56:59 -0400 Subject: [PATCH 10/16] docs(plan): #771 plan cycle 4 inline fixes (I1 raw dep.Name + I3 guard comment) Adversarial PASS cycle 4 with 3 micro-edit Importants. Apply inline: - I1 Option (b): Task 4 uses raw dep.Name for hash + lockfile key (matches un-normalized install at line 268). Reverts cycle-4's normalize-only-on-write asymmetry. - I3 Option (a): documentary note in installFromWfctlLockfile about dep auto-pin suppression under function-scope guard. - I2: dep entries always short-form (already documented behavior; existing runPluginInstall:257 pattern). --- docs/plans/2026-05-24-lockfile-deps.md | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/docs/plans/2026-05-24-lockfile-deps.md b/docs/plans/2026-05-24-lockfile-deps.md index 8cbee51f..bada4459 100644 --- a/docs/plans/2026-05-24-lockfile-deps.md +++ b/docs/plans/2026-05-24-lockfile-deps.md @@ -418,7 +418,15 @@ installSkipLockfileUpdate = true defer func() { installSkipLockfileUpdate = false }() ``` -In `cmd/wfctl/plugin_install_wfctllock.go`, find `installFromWfctlLockfile` (around line 27). At the TOP of the per-plugin loop body (before line 86's `installFromURL` call and before the line-99 fallback `runPluginInstall` call), add: +In `cmd/wfctl/plugin_install_wfctllock.go`, find `installFromWfctlLockfile` (around line 27). At the TOP of the per-plugin loop body (before line 86's `installFromURL` call and before the line-99 fallback `runPluginInstall` call), add a function-scope guard: + +```go +// Note (workflow#771): function-scope guard suppresses lockfile writes by +// inner install paths. Deps installed via the fallback runPluginInstall +// inherit this guard and are NOT auto-pinned to the lockfile. Users must +// explicitly install deps to add them — matches the "lockfile is what the +// user explicitly pinned" contract. +``` ```go installSkipLockfileUpdate = true @@ -506,18 +514,22 @@ In `cmd/wfctl/plugin_deps.go` after the existing `resolved[dep.Name] = depManife // Track dep in lockfile (workflow#771 Task 4). The chokepoint guard // inside updateLockfileWithChecksum (Task 1) suppresses writes when // running under an outer-frame installer (installFromLockfile etc.). - // Cycle-4 fix per reviewer CYC3-I1: normalize dep key for both hash - // path AND lockfile key — keeps dep entries consistent with parent - // entries (which use normalizePluginName per runPluginInstall line 257). - depKey := normalizePluginName(dep.Name) - depBinaryPath := filepath.Join(pluginDir, depKey, depKey) + // Cycle-4 reviewer I1 Option-(b): use raw dep.Name for ALL three sites + // (install dir, hash path, lockfile key) — matches the un-normalized + // install at line 268 (`installPluginFromManifest(pluginDir, dep.Name, ...)`). + // Normalizing only the lockfile-side without changing install-side + // produces hash-MISS warnings + empty checksums for long-form dep names. + // Parent (Task 5) keeps normalize because runPluginInstall:257 normalizes + // install-side too — symmetric for that path. Asymmetric across Task 4 vs 5 + // is a pre-existing convention difference, not regressed by this PR. + depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name) depChecksum := "" if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil { depChecksum = cs } else { fmt.Fprintf(os.Stderr, "warning: could not hash dep binary %s: %v (lockfile will have no checksum)\n", depBinaryPath, hashErr) } - updateLockfileWithChecksum(depKey, depManifest.Version, depManifest.Repository, "", depChecksum) + updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum) ``` Add `"path/filepath"` and `"os"` to the existing single import block in `plugin_deps.go` if not already present. From 809bcb890795f0dc31fc4d35b9d4bd73330fd2e9 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 04:57:59 -0400 Subject: [PATCH 11/16] chore: lock scope for lockfile-deps (alignment passed) --- docs/plans/2026-05-24-lockfile-deps.md | 2 +- docs/plans/2026-05-24-lockfile-deps.md.scope-lock | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 docs/plans/2026-05-24-lockfile-deps.md.scope-lock diff --git a/docs/plans/2026-05-24-lockfile-deps.md b/docs/plans/2026-05-24-lockfile-deps.md index bada4459..a15ce798 100644 --- a/docs/plans/2026-05-24-lockfile-deps.md +++ b/docs/plans/2026-05-24-lockfile-deps.md @@ -37,7 +37,7 @@ |------|-------|-------|--------| | 1 | feat(wfctl): lockfile dep tracking + always-track plain installs (workflow#771) | Task 1, Task 2, Task 3, Task 4, Task 5, Task 6 | feat/771-lockfile-deps | -**Status:** Draft +**Status:** Locked 2026-05-24T08:57:59Z ## Reviewer override (plan cycle 1 → cycle 2) diff --git a/docs/plans/2026-05-24-lockfile-deps.md.scope-lock b/docs/plans/2026-05-24-lockfile-deps.md.scope-lock new file mode 100644 index 00000000..af94427c --- /dev/null +++ b/docs/plans/2026-05-24-lockfile-deps.md.scope-lock @@ -0,0 +1 @@ +639ce3d1d266c442a6e7d4f16f177f30b643e7a53e14b09112a7928744a191eb From 5e7f2990f5b896c970d5c9f57516e8df2793881d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 05:00:53 -0400 Subject: [PATCH 12/16] feat(wfctl): chokepoint guard + new-format fan-out in updateLockfileWithChecksum (workflow#771 Task 1) --- cmd/wfctl/plugin_install.go | 12 ++++++ cmd/wfctl/plugin_install_lockfile_test.go | 49 +++++++++++++++++++++++ cmd/wfctl/plugin_lockfile.go | 45 ++++++++++++++++++++- 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 4d94c55f..903ff866 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -26,6 +26,18 @@ import ( // defaultDataDir is the default location for installed plugin binaries. const defaultDataDir = "data/plugins" +// installSkipLockfileUpdate suppresses ALL lockfile writes when set. Outer +// installers (installFromLockfile / installFromWfctlLockfile) hold the +// lockfile in memory and re-save it themselves; without this guard, inner +// install paths' lockfile writes would be silently overwritten by the +// outer re-save (workflow#771 cycle-5 chokepoint pattern). +// +// NOTE: package-level state. Tests touching this MUST NOT call t.Parallel() — +// cross-test flag leakage would silently break lockfile invariants. See +// design doc §"Top 3 doubts #2" for rationale on rejecting context.Context +// threading. +var installSkipLockfileUpdate bool + func runPluginSearch(args []string) error { fs := flag.NewFlagSet("plugin search", flag.ContinueOnError) cfgPath := fs.String("config", "", "Registry config file path") diff --git a/cmd/wfctl/plugin_install_lockfile_test.go b/cmd/wfctl/plugin_install_lockfile_test.go index f649ff48..70971f58 100644 --- a/cmd/wfctl/plugin_install_lockfile_test.go +++ b/cmd/wfctl/plugin_install_lockfile_test.go @@ -692,3 +692,52 @@ func TestInstallFromWfctlLockfile_PlatformSHA256IsCaseInsensitive(t *testing.T) t.Fatalf("installFromWfctlLockfile should accept uppercase platform checksum: %v", err) } } + +func TestUpdateLockfileWithChecksum_GuardSkips(t *testing.T) { + dir := t.TempDir() + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) + if err := os.WriteFile(".wfctl-lock.yaml", []byte("version: 1\nplugins: {}\n"), 0o600); err != nil { + t.Fatal(err) + } + installSkipLockfileUpdate = true + defer func() { installSkipLockfileUpdate = false }() + updateLockfileWithChecksum("foo", "1.0.0", "", "", "") + b, _ := os.ReadFile(wfctlLockPath) + if strings.Contains(string(b), "foo") { + t.Errorf("guard should have suppressed write; got: %s", b) + } +} + +func TestUpdateLockfileWithChecksum_NewFormatFanOut(t *testing.T) { + dir := t.TempDir() + prevWD, _ := os.Getwd() + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(prevWD) }) + if err := os.WriteFile(wfctlLockPath, []byte("version: 1\nplugins:\n bar:\n version: 0.1.0\n source: github.com/x/bar\n platforms:\n linux_amd64:\n url: https://example.com/bar\n sha256: deadbeef\n"), 0o600); err != nil { + t.Fatal(err) + } + updateLockfileWithChecksum("bar", "1.2.3", "github.com/x/bar-new", "", "feedface") + lf, err := config.LoadWfctlLockfile(".wfctl-lock.yaml") + if err != nil { + t.Fatal(err) + } + got := lf.Plugins["bar"] + if got.Version != "1.2.3" { + t.Errorf("Version = %q, want 1.2.3", got.Version) + } + if got.Source != "github.com/x/bar-new" { + t.Errorf("Source = %q, want github.com/x/bar-new", got.Source) + } + if len(got.Platforms) == 0 { + t.Errorf("Platforms should be preserved; got empty") + } + if got.Platforms["linux_amd64"].URL != "https://example.com/bar" { + t.Errorf("Platforms URL clobbered: %v", got.Platforms) + } +} diff --git a/cmd/wfctl/plugin_lockfile.go b/cmd/wfctl/plugin_lockfile.go index d31ddbba..7fcca9d7 100644 --- a/cmd/wfctl/plugin_lockfile.go +++ b/cmd/wfctl/plugin_lockfile.go @@ -139,15 +139,56 @@ func installFromLockfile(pluginDir, cfgPath string) error { return nil } +// mergeIntoNewFormatLockfile updates the new-format .wfctl-lock.yaml's +// Plugins[name] entry, preserving Platforms / Compatibility data while +// refreshing Version + Source. Returns true iff lockfile is v1 format +// (so caller can skip the legacy write path). +// +// name passed in normalized form. Helper handles existing entries keyed +// by long-form (e.g. "workflow-plugin-auth") by scanning for any key +// matching normalizePluginName. +func mergeIntoNewFormatLockfile(name, version, source string) bool { + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil || lf == nil || lf.Version == 0 { + return false + } + if lf.Plugins == nil { + lf.Plugins = make(map[string]config.WfctlLockPluginEntry) + } + // Lookup: exact match first, else scan for normalized-equivalent key. + key := name + if _, ok := lf.Plugins[name]; !ok { + for existingKey := range lf.Plugins { + if normalizePluginName(existingKey) == name { + key = existingKey + break + } + } + } + existing := lf.Plugins[key] + existing.Version = version + if source != "" { + existing.Source = source + } + lf.Plugins[key] = existing + _ = config.SaveWfctlLockfile(wfctlLockPath, lf) + return true +} + // updateLockfileWithChecksum adds or updates a plugin entry in .wfctl-lock.yaml // with SHA-256 checksum. The sha256Hash must be the hash of the installed binary, // not the download archive. // Silently no-ops if the lockfile cannot be read or written (install still succeeds). func updateLockfileWithChecksum(pluginName, version, repository, registry, sha256Hash string) { - if newLF, err := config.LoadWfctlLockfile(wfctlLockPath); err == nil && newLF.Version > 0 { + if installSkipLockfileUpdate { return } - + // V1 format takes precedence: if .wfctl-lock.yaml exists with version: 1, + // write ONLY to that path (preserves Platforms). Skip legacy save entirely. + if mergeIntoNewFormatLockfile(pluginName, version, repository) { + return + } + // Legacy fallback: no v1 lockfile present; write to legacy .wfctl.yaml plugins block. lf, err := loadPluginLockfile(wfctlLockPath) if err != nil { return From 6d73ddb90a709009ac13fe4e213781fb51a252ba Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 05:01:58 -0400 Subject: [PATCH 13/16] feat(wfctl): always-track plain install in lockfile (drop @version gate) (workflow#771 Task 2) --- cmd/wfctl/plugin_install.go | 22 +++--- cmd/wfctl/plugin_install_lockfile_test.go | 90 +++++++++++++++++++++++ 2 files changed, 101 insertions(+), 11 deletions(-) diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 903ff866..04d4d60d 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -264,18 +264,18 @@ func runPluginInstall(args []string) error { return err } - // Update .wfctl-lock.yaml lockfile if name@version was provided. - if _, ver := parseNameVersion(nameArg); ver != "" { - pluginName = normalizePluginName(pluginName) - binaryChecksum := "" - binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName) - if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { - binaryChecksum = cs - } else { - fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) - } - updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum) + // Update .wfctl-lock.yaml lockfile (workflow#771: always-track, gate removed). + // The chokepoint guard inside updateLockfileWithChecksum (Task 1) is responsible + // for suppressing writes during outer-frame installers. + pluginName = normalizePluginName(pluginName) + binaryChecksum := "" + binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName) + if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + binaryChecksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) } + updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum) return nil } diff --git a/cmd/wfctl/plugin_install_lockfile_test.go b/cmd/wfctl/plugin_install_lockfile_test.go index 70971f58..0af1d742 100644 --- a/cmd/wfctl/plugin_install_lockfile_test.go +++ b/cmd/wfctl/plugin_install_lockfile_test.go @@ -741,3 +741,93 @@ func TestUpdateLockfileWithChecksum_NewFormatFanOut(t *testing.T) { t.Errorf("Platforms URL clobbered: %v", got.Platforms) } } + +// TestRunPluginInstall_NoVersionTracksLockfile verifies that runPluginInstall +// invoked with a PLAIN name (no @version) still writes the resolved version +// into the new-format lockfile. Pre-Task-2 the @version gate would skip the +// lockfile write entirely for plain installs (workflow#771). +func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, ".wfctl-lock.yaml") + pluginDir := filepath.Join(dir, "plugins") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + + origWD, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + if err := os.Chdir(dir); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck + + const pluginName = "auth" + binaryContent := []byte("#!/bin/sh\necho auth\n") + tarball := buildPluginTarGz(t, pluginName, binaryContent, minimalPluginJSON(pluginName, "v1.2.3")) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/plugins/workflow-plugin-auth/manifest.json": + manifest := RegistryManifest{ + Name: pluginName, + Version: "v1.2.3", + Repository: "github.com/GoCodeAlone/workflow-plugin-auth", + Author: "tester", + Description: "test auth plugin", + Type: "external", + Tier: "community", + License: "MIT", + Downloads: []PluginDownload{ + { + OS: runtime.GOOS, + Arch: runtime.GOARCH, + URL: "http://" + r.Host + "/releases/download/v1.2.3/auth.tar.gz", + SHA256: sha256Hex(tarball), + }, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(manifest) + case "/releases/download/v1.2.3/auth.tar.gz": + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(tarball) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + regCfg := "registries:\n - name: test\n type: static\n url: " + srv.URL + "\n priority: 0\n" + if err := os.WriteFile(filepath.Join(dir, ".wfctl.yaml"), []byte(regCfg), 0o600); err != nil { + t.Fatalf("write registry config: %v", err) + } + // New-format empty lockfile so the chokepoint fan-out fires. + emptyLF := &config.WfctlLockfile{ + Version: 1, + GeneratedAt: time.Now(), + Plugins: map[string]config.WfctlLockPluginEntry{}, + } + if err := config.SaveWfctlLockfile(lockPath, emptyLF); err != nil { + t.Fatal(err) + } + + // PLAIN name — no @version. Pre-Task-2 this skips the lockfile write entirely. + if err := runPluginInstall([]string{"--plugin-dir", pluginDir, "workflow-plugin-auth"}); err != nil { + t.Fatalf("runPluginInstall: %v", err) + } + + loaded, err := config.LoadWfctlLockfile(lockPath) + if err != nil { + t.Fatalf("load lockfile: %v", err) + } + entry, ok := loaded.Plugins["auth"] + if !ok { + // mergeIntoNewFormatLockfile uses normalized name; auth was empty so key is "auth". + t.Fatalf("plain install should track lockfile entry; loaded.Plugins=%#v", loaded.Plugins) + } + if entry.Version != "v1.2.3" { + t.Errorf("plain install Version = %q, want v1.2.3", entry.Version) + } +} From 04c757fb1392c42a8e9de64e51e366e5322cc1f6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 05:04:40 -0400 Subject: [PATCH 14/16] feat(wfctl): set installSkipLockfileUpdate in outer-frame installers (workflow#771 Task 3) --- cmd/wfctl/plugin_install_lockfile_test.go | 104 ++++++++++++++++++++++ cmd/wfctl/plugin_install_wfctllock.go | 11 +++ cmd/wfctl/plugin_lockfile.go | 5 ++ 3 files changed, 120 insertions(+) diff --git a/cmd/wfctl/plugin_install_lockfile_test.go b/cmd/wfctl/plugin_install_lockfile_test.go index 0af1d742..c174736a 100644 --- a/cmd/wfctl/plugin_install_lockfile_test.go +++ b/cmd/wfctl/plugin_install_lockfile_test.go @@ -831,3 +831,107 @@ func TestRunPluginInstall_NoVersionTracksLockfile(t *testing.T) { t.Errorf("plain install Version = %q, want v1.2.3", entry.Version) } } + +// TestInstallFromLockfile_NoClobberInvariant verifies the outer-frame guard: +// when installFromLockfile drives the install loop, inner runPluginInstall +// (now Task-2 always-tracks) must NOT mutate the on-disk lockfile — +// otherwise inner writes would clobber the pinned entries before the outer +// checksum verification completes. +func TestInstallFromLockfile_NoClobberInvariant(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, ".wfctl-lock.yaml") + pluginDir := filepath.Join(dir, "plugins") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + + origWD, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + if err := os.Chdir(dir); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck + + const pluginName = "auth" + binaryContent := []byte("#!/bin/sh\necho auth\n") + tarball := buildPluginTarGz(t, pluginName, binaryContent, minimalPluginJSON(pluginName, "v1.2.3")) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/plugins/workflow-plugin-auth/manifest.json": + manifest := RegistryManifest{ + Name: pluginName, + Version: "v1.2.3", + Repository: "github.com/GoCodeAlone/workflow-plugin-auth", + Author: "tester", + Description: "test auth plugin", + Type: "external", + Tier: "community", + License: "MIT", + Downloads: []PluginDownload{ + { + OS: runtime.GOOS, + Arch: runtime.GOARCH, + URL: "http://" + r.Host + "/releases/download/v1.2.3/auth.tar.gz", + SHA256: sha256Hex(tarball), + }, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(manifest) + case "/releases/download/v1.2.3/auth.tar.gz": + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(tarball) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + regCfg := "registries:\n - name: test\n type: static\n url: " + srv.URL + "\n priority: 0\n" + if err := os.WriteFile(filepath.Join(dir, ".wfctl.yaml"), []byte(regCfg), 0o600); err != nil { + t.Fatalf("write registry config: %v", err) + } + + // Pinned new-format lockfile entry with sentinel Source so we can detect + // inner-install clobber (inner would overwrite Source with the manifest's repo). + lf := &config.WfctlLockfile{ + Version: 1, + GeneratedAt: time.Now(), + Plugins: map[string]config.WfctlLockPluginEntry{ + "workflow-plugin-auth": { + Version: "v1.2.3", + Source: "github.com/PINNED/auth-sentinel", + SHA256: strings.Repeat("0", 64), + Platforms: map[string]config.WfctlLockPlatform{ + currentPlatformKey(): { + URL: "http://" + strings.TrimPrefix(srv.URL, "http://") + "/releases/download/v1.2.3/auth.tar.gz", + SHA256: sha256Hex(tarball), + }, + }, + }, + }, + } + if err := config.SaveWfctlLockfile(lockPath, lf); err != nil { + t.Fatal(err) + } + + if err := installFromLockfile(pluginDir, ""); err != nil { + t.Fatalf("installFromLockfile: %v", err) + } + + loaded, err := config.LoadWfctlLockfile(lockPath) + if err != nil { + t.Fatalf("load lockfile: %v", err) + } + entry := loaded.Plugins["workflow-plugin-auth"] + if entry.Source != "github.com/PINNED/auth-sentinel" { + t.Errorf("outer-frame guard missing: Source clobbered by inner install. got=%q want=github.com/PINNED/auth-sentinel", entry.Source) + } + platform := entry.Platforms[currentPlatformKey()] + if platform.URL == "" { + t.Errorf("outer-frame guard missing: Platforms wiped by inner install. got=%#v", entry.Platforms) + } +} diff --git a/cmd/wfctl/plugin_install_wfctllock.go b/cmd/wfctl/plugin_install_wfctllock.go index 1b8e67c8..e5fced80 100644 --- a/cmd/wfctl/plugin_install_wfctllock.go +++ b/cmd/wfctl/plugin_install_wfctllock.go @@ -30,6 +30,17 @@ func installFromWfctlLockfile(pluginDirVal, lockPath string, lf *config.WfctlLoc return nil } + // Function-scope guard (workflow#771 Task 3): suppress lockfile writes from + // inner install paths (installFromURL + fallback runPluginInstall) so that + // platform metadata + pinned entries remain authoritative. Deps installed + // via the fallback runPluginInstall inherit this guard and are NOT + // auto-pinned to the lockfile — matches the "lockfile is what the user + // explicitly pinned" contract. Cycle-3 fix per CYC2-C2: function-scope + // because per-iteration anon-func wrappers would break the loop's 6× + // `continue` statements (continue not allowed across func literal). + installSkipLockfileUpdate = true + defer func() { installSkipLockfileUpdate = false }() + if lockPath != "" { scrubbed := scrubbedWfctlLockfileTopLevelSHA256(lf) if err := config.SaveWfctlLockfile(lockPath, scrubbed); err != nil { diff --git a/cmd/wfctl/plugin_lockfile.go b/cmd/wfctl/plugin_lockfile.go index 7fcca9d7..1ba88f42 100644 --- a/cmd/wfctl/plugin_lockfile.go +++ b/cmd/wfctl/plugin_lockfile.go @@ -102,6 +102,11 @@ func installFromLockfile(pluginDir, cfgPath string) error { fmt.Println("Run 'wfctl plugin install @' to install and pin a plugin.") return nil } + // Function-scope guard (workflow#771 Task 3): suppress lockfile writes from + // inner runPluginInstall during the loop, so the on-disk pinned entries are + // not clobbered before checksum verification completes. + installSkipLockfileUpdate = true + defer func() { installSkipLockfileUpdate = false }() var failed []string for name, entry := range lf.Plugins { fmt.Fprintf(os.Stderr, "Installing %s %s...\n", name, entry.Version) From 82dbf49bedc50e84b1d424efe65df1cadab7b465 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 05:06:18 -0400 Subject: [PATCH 15/16] feat(wfctl): track transitive deps in lockfile via resolveDependencies (workflow#771 Task 4) --- cmd/wfctl/plugin_deps.go | 20 +++++++ cmd/wfctl/plugin_deps_test.go | 99 +++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/cmd/wfctl/plugin_deps.go b/cmd/wfctl/plugin_deps.go index e8976fc0..33301198 100644 --- a/cmd/wfctl/plugin_deps.go +++ b/cmd/wfctl/plugin_deps.go @@ -269,6 +269,26 @@ func resolveDependencies( return fmt.Errorf("install dependency %q of %q: %w", dep.Name, pluginName, err) } resolved[dep.Name] = depManifest.Version + + // Track dep in lockfile (workflow#771 Task 4). The chokepoint guard + // inside updateLockfileWithChecksum (Task 1) suppresses writes when + // running under an outer-frame installer (installFromLockfile etc.). + // Cycle-4 reviewer I1 Option-(b): use raw dep.Name for ALL three sites + // (install dir, hash path, lockfile key) — matches the un-normalized + // install above (`installPluginFromManifest(pluginDir, dep.Name, ...)`). + // Normalizing only the lockfile-side without changing install-side + // produces hash-MISS warnings + empty checksums for long-form dep names. + // Parent (Task 5) keeps normalize because runPluginInstall:257 normalizes + // install-side too — symmetric for that path. Asymmetric across Task 4 vs 5 + // is a pre-existing convention difference, not regressed by this PR. + depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name) + depChecksum := "" + if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil { + depChecksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash dep binary %s: %v (lockfile will have no checksum)\n", depBinaryPath, hashErr) + } + updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum) } return nil } diff --git a/cmd/wfctl/plugin_deps_test.go b/cmd/wfctl/plugin_deps_test.go index ccd6a7f9..f705b835 100644 --- a/cmd/wfctl/plugin_deps_test.go +++ b/cmd/wfctl/plugin_deps_test.go @@ -7,7 +7,11 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "testing" + "time" + + "github.com/GoCodeAlone/workflow/config" ) // TestCompareSemverConstraints verifies semver comparison used in version constraint checks. @@ -341,6 +345,101 @@ func TestPluginInstall_ResolveDependencies(t *testing.T) { } } +// TestResolveDependencies_TracksDepsInLockfile verifies workflow#771 Task 4: +// when resolveDependencies installs a transitive dep, the lockfile receives an +// entry for that dep (provided no outer-frame installer suppressed writes). +func TestResolveDependencies_TracksDepsInLockfile(t *testing.T) { + pluginDir := t.TempDir() + + origWD, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + cwd := t.TempDir() + if err := os.Chdir(cwd); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck + + // Seed an empty v1 lockfile so the chokepoint fan-out fires. + emptyLF := &config.WfctlLockfile{ + Version: 1, + GeneratedAt: time.Now(), + Plugins: map[string]config.WfctlLockPluginEntry{}, + } + if err := config.SaveWfctlLockfile(wfctlLockPath, emptyLF); err != nil { + t.Fatal(err) + } + + const depName = "depa" + binaryContent := []byte("#!/bin/sh\necho depa\n") + pjContent := minimalPluginJSON(depName, "0.5.0") + tarball := buildPluginTarGz(t, depName, binaryContent, pjContent) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/plugins/" + depName + "/manifest.json": + manifest := RegistryManifest{ + Name: depName, + Version: "0.5.0", + Repository: "github.com/x/" + depName, + Author: "tester", + Description: "depa", + Type: "external", + Tier: "community", + License: "MIT", + Downloads: []PluginDownload{ + { + OS: runtime.GOOS, + Arch: runtime.GOARCH, + URL: "http://" + r.Host + "/download/" + depName + ".tar.gz", + SHA256: sha256Hex(tarball), + }, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(manifest) + case "/download/" + depName + ".tar.gz": + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(tarball) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + cfgFile := writeTestRegistryConfig(t, srv.URL) + + parentManifest := &RegistryManifest{ + Name: "parent", + Version: "1.0.0", + Dependencies: []PluginDependency{ + {Name: depName, MinVersion: "0.1.0"}, + }, + } + + resolved := make(map[string]string) + if err := resolveDependencies("parent", parentManifest, pluginDir, cfgFile, []string{}, resolved); err != nil { + t.Fatalf("resolveDependencies: %v", err) + } + if resolved[depName] != "0.5.0" { + t.Errorf("resolved[%s] = %q, want 0.5.0", depName, resolved[depName]) + } + + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatalf("load lockfile: %v", err) + } + entry, ok := lf.Plugins[depName] + if !ok { + t.Fatalf("dep not tracked in lockfile; lf.Plugins=%#v", lf.Plugins) + } + if entry.Version != "0.5.0" { + t.Errorf("dep entry Version = %q, want 0.5.0", entry.Version) + } +} + // writeTestRegistryConfig writes a minimal registry YAML config to a temp file // pointing at the given static URL, and returns the file path. func writeTestRegistryConfig(t *testing.T, baseURL string) string { From a17be3867b7144513526e55b459cbd1774941bba Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 24 May 2026 05:07:23 -0400 Subject: [PATCH 16/16] feat(wfctl): track --from-config parent in lockfile via installPluginReqDirect (workflow#771 Task 5) --- cmd/wfctl/plugin_deps.go | 22 ++++++++- cmd/wfctl/plugin_deps_test.go | 92 +++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/cmd/wfctl/plugin_deps.go b/cmd/wfctl/plugin_deps.go index 33301198..48e01184 100644 --- a/cmd/wfctl/plugin_deps.go +++ b/cmd/wfctl/plugin_deps.go @@ -108,7 +108,27 @@ func installPluginReqDirect(pluginDir, registryCfgPath string, req config.Plugin } } - return installPluginFromManifest(pluginDir, pluginName, manifest, req.Verify, false) + if err := installPluginFromManifest(pluginDir, pluginName, manifest, req.Verify, false); err != nil { + return err + } + + // Track parent in lockfile (workflow#771 Task 5). Closes the asymmetry + // where --from-config dep installs were tracked via Task 4 but parent + // installs via this path were not. + // Cycle-3 fix per reviewer CYC2-I1: use the function's NORMALIZED pluginName + // (already computed at line 87 via normalizePluginName(rawName)), NOT raw + // req.Name. installPluginFromManifest installs at pluginDir//; + // raw req.Name "workflow-plugin-auth" would hash-miss at pluginDir/workflow-plugin-auth/... + // and produce an asymmetric lockfile key vs runPluginInstall's writes. + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + checksum := "" + if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil { + checksum = cs + } else { + fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr) + } + updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, "", checksum) + return nil } // runPluginDeps lists dependencies for a plugin without installing them. diff --git a/cmd/wfctl/plugin_deps_test.go b/cmd/wfctl/plugin_deps_test.go index f705b835..f2ae6df0 100644 --- a/cmd/wfctl/plugin_deps_test.go +++ b/cmd/wfctl/plugin_deps_test.go @@ -440,6 +440,98 @@ func TestResolveDependencies_TracksDepsInLockfile(t *testing.T) { } } +// TestInstallPluginReqDirect_TracksParentInLockfile verifies workflow#771 Task 5: +// installPluginReqDirect writes a lockfile entry for the parent plugin (closing +// the asymmetry where --from-config dep installs were tracked via Task 4 but +// parent installs via this path were not). +func TestInstallPluginReqDirect_TracksParentInLockfile(t *testing.T) { + pluginDir := t.TempDir() + + origWD, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + cwd := t.TempDir() + if err := os.Chdir(cwd); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck + + emptyLF := &config.WfctlLockfile{ + Version: 1, + GeneratedAt: time.Now(), + Plugins: map[string]config.WfctlLockPluginEntry{}, + } + if err := config.SaveWfctlLockfile(wfctlLockPath, emptyLF); err != nil { + t.Fatal(err) + } + + // Parent plugin uses long-form name to verify normalize-to-lockfile-key. + const parentLong = "workflow-plugin-fromcfg" + const parentShort = "fromcfg" + binaryContent := []byte("#!/bin/sh\necho fromcfg\n") + pjContent := minimalPluginJSON(parentShort, "1.5.0") + tarball := buildPluginTarGz(t, parentShort, binaryContent, pjContent) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/plugins/" + parentLong + "/manifest.json": + manifest := RegistryManifest{ + Name: parentShort, + Version: "1.5.0", + Repository: "github.com/x/" + parentShort, + Author: "tester", + Description: "fromcfg", + Type: "external", + Tier: "community", + License: "MIT", + Downloads: []PluginDownload{ + { + OS: runtime.GOOS, + Arch: runtime.GOARCH, + URL: "http://" + r.Host + "/download/" + parentShort + ".tar.gz", + SHA256: sha256Hex(tarball), + }, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(manifest) + case "/download/" + parentShort + ".tar.gz": + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(tarball) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + cfgFile := writeTestRegistryConfig(t, srv.URL) + + req := config.PluginRequirement{ + Name: parentLong, + Version: "1.5.0", + } + + if err := installPluginReqDirect(pluginDir, cfgFile, req); err != nil { + t.Fatalf("installPluginReqDirect: %v", err) + } + + lf, err := config.LoadWfctlLockfile(wfctlLockPath) + if err != nil { + t.Fatalf("load lockfile: %v", err) + } + // installPluginReqDirect normalizes via normalizePluginName at line 87; + // the lockfile entry key is the NORMALIZED form ("fromcfg"). + entry, ok := lf.Plugins[parentShort] + if !ok { + t.Fatalf("parent not tracked in lockfile (normalized key %q missing); lf.Plugins=%#v", parentShort, lf.Plugins) + } + if entry.Version != "1.5.0" { + t.Errorf("parent entry Version = %q, want 1.5.0", entry.Version) + } +} + // writeTestRegistryConfig writes a minimal registry YAML config to a temp file // pointing at the given static URL, and returns the file path. func writeTestRegistryConfig(t *testing.T, baseURL string) string {