← back to Cli Printing Press
fix(cli): normalize lock name so slug and binary form share lock file (#493)
820ba7b1fb543ee9b8c663c3f39b5a7f284822db · 2026-05-01 22:11:04 -0700 · Trevin Chow
* fix(cli): normalize lock name so slug and binary form share lock file
LockFilePath used the cliName argument verbatim, so the build (which
acquires the lock as "<api>-pp-cli") and the polish skill's lock-status
safety net (which derives the name from a library directory's basename,
the slug) wrote and read different files. When polish was invoked
mid-pipeline (Phase 5.5) with the slug, the safety check returned
held=false and polish silently operated on the prior run's stale library
copy.
Two layers, both on the same root cause:
- internal/pipeline/lock.go: add normalizeLockName (uses
naming.IsCLIDirName / naming.CLI) and route LockFilePath through it.
All other lock helpers go through LockFilePath, so they inherit the
normalization. Two lock_test.go cases pin both directions
(slug-then-binary and binary-then-slug observe the same lock).
- skills/printing-press/SKILL.md (Phase 5.5) and
skills/printing-press-polish/SKILL.md (resolver section): explain why
the mid-pipeline caller must pass $CLI_WORK_DIR (absolute working-dir
path) and not the slug. Phase 5.5 fires before Phase 5.6 promotes the
working CLI, so $PRESS_LIBRARY/<slug>/ either doesn't exist or holds
the prior run.
Fixes #483
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cli): validate cli name at lock entry to prevent path traversal
LockFilePath joined cliName into a filesystem path via filepath.Join, so
a value like "../foo" passed via `printing-press lock --cli ...` would
escape LocksDir() once the ".." was resolved. The hole pre-dated the
slug/binary normalization shipped in the previous commit, but normalization
alone did not close it: the same filepath.Join still ran on the raw input.
AcquireLock, UpdateLock, ReleaseLock, and PromoteWorkingCLI now return an
"invalid cli name" error when validateCLIName (delegating to
naming.IsValidLibraryDirName) rejects the input. LockStatus has no error
channel, so it returns a zero result for invalid input — safe behavior at
that boundary. LockFilePath itself stays a pure path computation; its
contract now explicitly assumes the caller has already validated.
Also drops a load-bearing skill reference from LockFilePath's doc comment
per AGENTS.md "Code comments must be self-contained" — the rationale lives
in commits and the PR description, not code.
Adds two table tests:
- TestLockFilePath_NormalizationEdgeCases pins the normalization output for
bare slugs, binary names, legacy -cli, rerun forms, and the empty
degenerate, so future naming-helper refactors can't drift lock-file paths.
- TestLockHelpers_RejectInvalidCLIName covers six traversal/format payloads
across all four lock entry points.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(cli): drop ticket reference from lock test comment
Per AGENTS.md "no dates, incidents, or ticket numbers in code comments";
the rationale lives in the commit message and PR description.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files touched
M internal/pipeline/lock.goM internal/pipeline/lock_test.goM skills/printing-press-polish/SKILL.mdM skills/printing-press/SKILL.md
Diff
commit 820ba7b1fb543ee9b8c663c3f39b5a7f284822db
Author: Trevin Chow <trevin@trevinchow.com>
Date: Fri May 1 22:11:04 2026 -0700
fix(cli): normalize lock name so slug and binary form share lock file (#493)
* fix(cli): normalize lock name so slug and binary form share lock file
LockFilePath used the cliName argument verbatim, so the build (which
acquires the lock as "<api>-pp-cli") and the polish skill's lock-status
safety net (which derives the name from a library directory's basename,
the slug) wrote and read different files. When polish was invoked
mid-pipeline (Phase 5.5) with the slug, the safety check returned
held=false and polish silently operated on the prior run's stale library
copy.
Two layers, both on the same root cause:
- internal/pipeline/lock.go: add normalizeLockName (uses
naming.IsCLIDirName / naming.CLI) and route LockFilePath through it.
All other lock helpers go through LockFilePath, so they inherit the
normalization. Two lock_test.go cases pin both directions
(slug-then-binary and binary-then-slug observe the same lock).
- skills/printing-press/SKILL.md (Phase 5.5) and
skills/printing-press-polish/SKILL.md (resolver section): explain why
the mid-pipeline caller must pass $CLI_WORK_DIR (absolute working-dir
path) and not the slug. Phase 5.5 fires before Phase 5.6 promotes the
working CLI, so $PRESS_LIBRARY/<slug>/ either doesn't exist or holds
the prior run.
Fixes #483
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cli): validate cli name at lock entry to prevent path traversal
LockFilePath joined cliName into a filesystem path via filepath.Join, so
a value like "../foo" passed via `printing-press lock --cli ...` would
escape LocksDir() once the ".." was resolved. The hole pre-dated the
slug/binary normalization shipped in the previous commit, but normalization
alone did not close it: the same filepath.Join still ran on the raw input.
AcquireLock, UpdateLock, ReleaseLock, and PromoteWorkingCLI now return an
"invalid cli name" error when validateCLIName (delegating to
naming.IsValidLibraryDirName) rejects the input. LockStatus has no error
channel, so it returns a zero result for invalid input — safe behavior at
that boundary. LockFilePath itself stays a pure path computation; its
contract now explicitly assumes the caller has already validated.
Also drops a load-bearing skill reference from LockFilePath's doc comment
per AGENTS.md "Code comments must be self-contained" — the rationale lives
in commits and the PR description, not code.
Adds two table tests:
- TestLockFilePath_NormalizationEdgeCases pins the normalization output for
bare slugs, binary names, legacy -cli, rerun forms, and the empty
degenerate, so future naming-helper refactors can't drift lock-file paths.
- TestLockHelpers_RejectInvalidCLIName covers six traversal/format payloads
across all four lock entry points.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(cli): drop ticket reference from lock test comment
Per AGENTS.md "no dates, incidents, or ticket numbers in code comments";
the rationale lives in the commit message and PR description.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
internal/pipeline/lock.go | 46 ++++++++++++++--
internal/pipeline/lock_test.go | 99 +++++++++++++++++++++++++++++++++++
skills/printing-press-polish/SKILL.md | 7 +++
skills/printing-press/SKILL.md | 2 +
4 files changed, 151 insertions(+), 3 deletions(-)
diff --git a/internal/pipeline/lock.go b/internal/pipeline/lock.go
index 9c59cf29..5626597e 100644
--- a/internal/pipeline/lock.go
+++ b/internal/pipeline/lock.go
@@ -42,15 +42,40 @@ func LocksDir() string {
return filepath.Join(PressHome(), locksDir)
}
-// LockFilePath returns the lock file path for a given CLI name.
+// LockFilePath returns the lock file path for a given CLI name. Callers may
+// pass either the slug or the binary name; both forms resolve to the same
+// file. Caller is responsible for validating the name first via
+// validateCLIName — LockFilePath itself does not check.
func LockFilePath(cliName string) string {
- return filepath.Join(LocksDir(), cliName+".lock")
+ return filepath.Join(LocksDir(), normalizeLockName(cliName)+".lock")
+}
+
+func normalizeLockName(cliName string) string {
+ if naming.IsCLIDirName(cliName) {
+ return cliName
+ }
+ return naming.CLI(cliName)
+}
+
+// validateCLIName rejects names that would escape LocksDir() once joined as
+// a filename. naming.IsValidLibraryDirName covers path separators, "..",
+// NUL, dotfile prefixes, and non-slug shapes. The lock helpers below call
+// this at entry so a malicious or buggy --cli value can never reach
+// filepath.Join with traversal characters intact.
+func validateCLIName(cliName string) error {
+ if !naming.IsValidLibraryDirName(cliName) {
+ return fmt.Errorf("invalid cli name %q", cliName)
+ }
+ return nil
}
// AcquireLock attempts to acquire a build lock for the given CLI.
// It auto-reclaims stale locks. If force is true, it overrides even fresh
// locks held by a different scope.
func AcquireLock(cliName, scope string, force bool) (*LockState, error) {
+ if err := validateCLIName(cliName); err != nil {
+ return nil, err
+ }
lockPath := LockFilePath(cliName)
if err := os.MkdirAll(LocksDir(), 0o755); err != nil {
@@ -112,6 +137,9 @@ func AcquireLock(cliName, scope string, force bool) (*LockState, error) {
// UpdateLock refreshes the heartbeat and phase of an existing lock.
func UpdateLock(cliName, phase string) error {
+ if err := validateCLIName(cliName); err != nil {
+ return err
+ }
lockPath := LockFilePath(cliName)
existing, err := readLock(lockPath)
@@ -127,10 +155,16 @@ func UpdateLock(cliName, phase string) error {
}
// LockStatus returns the current lock state for a CLI, including whether
-// a completed CLI exists in the library.
+// a completed CLI exists in the library. An invalid cliName returns the
+// zero result (no lock held, no library CLI) — safe behavior at this
+// boundary since the function has no error channel.
func LockStatus(cliName string) LockStatusResult {
result := LockStatusResult{}
+ if err := validateCLIName(cliName); err != nil {
+ return result
+ }
+
// Check library for completed CLI (slug-keyed directory).
slug := naming.TrimCLISuffix(cliName)
libDir := filepath.Join(PublishedLibraryRoot(), slug)
@@ -161,6 +195,9 @@ func LockStatus(cliName string) LockStatusResult {
// ReleaseLock removes the lock file for a CLI. It is idempotent.
func ReleaseLock(cliName string) error {
+ if err := validateCLIName(cliName); err != nil {
+ return err
+ }
lockPath := LockFilePath(cliName)
err := os.Remove(lockPath)
if err != nil && !os.IsNotExist(err) {
@@ -174,6 +211,9 @@ func ReleaseLock(cliName string) error {
// Uses a staging directory with atomic swap so the previous library copy
// survives if any step fails.
func PromoteWorkingCLI(cliName, workingDir string, state *PipelineState) error {
+ if err := validateCLIName(cliName); err != nil {
+ return err
+ }
if workingDir == "" {
return fmt.Errorf("working directory is empty")
}
diff --git a/internal/pipeline/lock_test.go b/internal/pipeline/lock_test.go
index 2442e11f..e50d4a0e 100644
--- a/internal/pipeline/lock_test.go
+++ b/internal/pipeline/lock_test.go
@@ -211,6 +211,105 @@ func TestReleaseLock_Idempotent(t *testing.T) {
assert.NoError(t, err)
}
+// LockFilePath must produce the same path regardless of whether the caller
+// passes the slug ("notion") or the binary name ("notion-pp-cli"). A mixed-
+// form caller would otherwise silently miss an active lock acquired under
+// the other form.
+func TestLockFilePath_NormalizesSlugAndBinaryName(t *testing.T) {
+ setupLockTest(t)
+
+ assert.Equal(t, LockFilePath("notion-pp-cli"), LockFilePath("notion"))
+}
+
+// Pin LockFilePath's normalization output across the name-form edge cases
+// callers actually produce: bare slugs, binary names, legacy -cli suffix,
+// rerun forms (numeric suffix on either side of -pp-cli), and the empty
+// degenerate. These pin current behavior so a future refactor of the
+// underlying naming helpers can't quietly drift the lock-file naming.
+func TestLockFilePath_NormalizationEdgeCases(t *testing.T) {
+ setupLockTest(t)
+
+ cases := []struct {
+ name string
+ input string
+ expected string
+ }{
+ {"bare slug appends -pp-cli", "foo", "foo-pp-cli.lock"},
+ {"binary name kept verbatim", "foo-pp-cli", "foo-pp-cli.lock"},
+ {"legacy -cli suffix kept verbatim", "foo-cli", "foo-cli.lock"},
+ {"rerun binary form (suffix-then-number) kept", "foo-pp-cli-2", "foo-pp-cli-2.lock"},
+ {"rerun library form gets -pp-cli appended", "foo-2", "foo-2-pp-cli.lock"},
+ {"empty string degenerates to bare suffix", "", "-pp-cli.lock"},
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ assert.Equal(t, tc.expected, filepath.Base(LockFilePath(tc.input)))
+ })
+ }
+}
+
+// Lock helpers must reject cliName values that would escape LocksDir() or
+// otherwise behave as filesystem-traversal payloads. Without this guard a
+// caller passing "../foo" via `printing-press lock --cli ...` would write
+// the lock file outside LocksDir() once filepath.Join resolved the "..".
+func TestLockHelpers_RejectInvalidCLIName(t *testing.T) {
+ setupLockTest(t)
+
+ cases := []struct {
+ name string
+ input string
+ }{
+ {"path traversal with dotdot", "../foo"},
+ {"forward slash", "foo/bar"},
+ {"dotfile prefix", ".hidden"},
+ {"embedded dotdot", "foo..bar"},
+ {"empty string", ""},
+ {"uppercase rejected by slug grammar", "FooBar"},
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ _, err := AcquireLock(tc.input, "scope-1", false)
+ assert.Error(t, err, "AcquireLock must reject %q", tc.input)
+
+ err = UpdateLock(tc.input, "build")
+ assert.Error(t, err, "UpdateLock must reject %q", tc.input)
+
+ err = ReleaseLock(tc.input)
+ assert.Error(t, err, "ReleaseLock must reject %q", tc.input)
+
+ status := LockStatus(tc.input)
+ assert.False(t, status.Held, "LockStatus must return zero result for %q", tc.input)
+ assert.False(t, status.HasCLI, "LockStatus must return zero result for %q", tc.input)
+ })
+ }
+}
+
+// AcquireLock with the binary name and LockStatus with the slug (or vice
+// versa) must observe the same lock. This is the polish-skill safety net:
+// when polish is invoked mid-pipeline, its lock check needs to detect the
+// build's lock regardless of which name form polish derives.
+func TestLockStatus_SeesLockAcquiredByOtherNameForm(t *testing.T) {
+ setupLockTest(t)
+
+ // Build acquires under the binary name.
+ _, err := AcquireLock("notion-pp-cli", "build-scope", false)
+ require.NoError(t, err)
+
+ // Polish queries with the slug (basename of $PRESS_LIBRARY/notion).
+ status := LockStatus("notion")
+ assert.True(t, status.Held, "lock acquired as binary name must be visible by slug")
+ assert.Equal(t, "build-scope", status.Scope)
+
+ // And the reverse: lock acquired by slug must be visible by binary name.
+ require.NoError(t, ReleaseLock("notion-pp-cli"))
+ _, err = AcquireLock("notion", "polish-scope", false)
+ require.NoError(t, err)
+
+ status = LockStatus("notion-pp-cli")
+ assert.True(t, status.Held, "lock acquired by slug must be visible by binary name")
+ assert.Equal(t, "polish-scope", status.Scope)
+}
+
func TestPromoteWorkingCLI(t *testing.T) {
tmp := t.TempDir()
t.Setenv("PRINTING_PRESS_HOME", tmp)
diff --git a/skills/printing-press-polish/SKILL.md b/skills/printing-press-polish/SKILL.md
index 6d700db9..0ede58a7 100644
--- a/skills/printing-press-polish/SKILL.md
+++ b/skills/printing-press-polish/SKILL.md
@@ -77,6 +77,13 @@ Resolution order:
3. If arg has `-pp-cli` suffix, strip it and try `$PRESS_LIBRARY/<slug>` (e.g., `redfin-pp-cli` → `redfin`)
4. Fuzzy search: `ls $PRESS_LIBRARY/ | grep -i <arg>` for close matches
+**Caller scenarios.** Polish has two callers and they pass different argument forms:
+
+- **Standalone (user-invoked, `/printing-press-polish redfin`).** The arg is a slug or binary name; resolution lands on `$PRESS_LIBRARY/<slug>/`. This is the published copy and the right target.
+- **Mid-pipeline (main printing-press skill Phase 5.5).** The arg is `$CLI_WORK_DIR` — an absolute path to `~/printing-press/.runstate/.../runs/.../working/<api>-pp-cli/`. Resolution must hit rule 1. **Do not paraphrase this to the slug** — Phase 5.5 fires before the working CLI is promoted, so `$PRESS_LIBRARY/<slug>/` either doesn't exist or holds the *prior* run's stale CLI.
+
+The lock-status check in the next code block is the safety net for the mid-pipeline scenario: if a build lock is held for this CLI (under either name form), polish refuses to run. `printing-press lock` normalizes slug ↔ binary-name internally, so the check works regardless of which form the basename produces.
+
If no match or multiple matches, present via `AskUserQuestion`. Show at most 4
matches sorted by modification time (most recent first) with human-friendly
relative timestamps (e.g., "generated 2 hours ago").
diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index 7094bcdf..6a23133b 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -2327,6 +2327,8 @@ Skill(
)
```
+**Pass `$CLI_WORK_DIR` (the absolute working-dir path), not the API slug.** Phase 5.5 fires before Phase 5.6 promotes the working CLI to the library, so `$PRESS_LIBRARY/<slug>/` either doesn't exist yet or contains the *prior* run's CLI. If you paraphrase the args to the slug (e.g., `args: "producthunt"`), polish silently operates on the stale library copy.
+
The polish skill runs the full diagnostic-fix-rediagnose loop including MCP tool quality polish (via `printing-press tools-audit` plus the playbook at `references/tools-polish.md`) and ends its response with a `---POLISH-RESULT---` block containing scorecard/verify/tools-audit before/after, fixes applied, and a ship recommendation.
Parse the result block. Display the delta to the user:
← 67597dbb fix(cli): preserve Unicode in OpenAPI display_name (#492)
·
back to Cli Printing Press
·
chore(main): release 3.4.1 (#494) a97b18c1 →