From 870c2d69d5a5b06c1bb79dba59419809d19c12ae Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 21 May 2026 19:36:14 -0500 Subject: [PATCH] Consolidated addition, removal, and listing of locks under a single narratio locks command --- docs/cli.md | 53 ++++--- docs/config.md | 2 +- docs/operations.md | 8 +- docs/roadmap/operations.md | 9 +- internal/app/commands.go | 6 +- internal/app/operator_helpers.go | 76 ++++++---- internal/app/operator_helpers_test.go | 200 ++++++++++++++++++++++++-- 7 files changed, 282 insertions(+), 72 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 400a934..6d23b17 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -23,9 +23,7 @@ Implemented commands: - `session validate`: run read-only preflight checks for a session. - `session init`: create local or remote `session.yml`. - `artifacts list`: list effective artifact source IDs. -- `locks`: list effective archive promotion locks. -- `lock`: add or update a remote session lock. -- `unlock`: remove a remote session lock. +- `locks`: list, add, and remove archive promotion locks. Unknown commands print usage and exit non-zero. @@ -136,17 +134,17 @@ Valid stage names: - `--previous-session-id ` - `--remote`: check promoted remote object availability. -### `locks`, `lock`, `unlock` +### `locks` -- `--config ` -- `--campaign ` -- `--session ` -- `--session-id ` -- `--previous-session-id ` -- `lock ` positional source ID. -- `lock --reason ` optional remote lock reason. -- `lock --force` updates an existing remote lock. -- `unlock ` positional source ID. +- `--session-id `: required for list, add, and remove. +- `--config `: optional explicit `pipeline.yml` path. +- `--campaign `: optional explicit `campaign.yml` path. +- `--session `: optional explicit `session.yml` path. +- `--previous-session-id `: optional session template value. +- `add `: add a remote lock for one artifact or transcript source. +- `add --reason `: record an optional remote lock reason. +- `add --force`: update the reason for an existing remote lock. +- `remove `: remove one remote lock. ## Command Reference @@ -291,25 +289,34 @@ narratio artifacts list [--config ] [--campaign ] [- `--remote` checks promoted top-level object availability through the storage adapter. -### `locks`, `lock`, and `unlock` +### `locks` Purpose: -- Inspect and mutate source-based archive promotion locks. +- Inspect and mutate source-based archive promotion locks for one session. Syntax: ```bash -narratio locks [--config ] [--campaign ] [--session ] [--session-id ] -narratio lock [flags] -narratio unlock [flags] +narratio locks --session-id +narratio locks add --session-id [--reason ] [--force] +narratio locks remove --session-id ``` Behavior: -- static locks from `pipeline.archive.locks` and remote locks from `{session_prefix}/locks.yml` are merged. -- static locks win when sources duplicate remote locks. -- `lock` writes or updates only remote locks. -- `unlock` removes only remote locks and cannot remove static pipeline locks. -- `lock --force` is required to update an existing remote lock reason. +- `--session-id` is required for list, add, and remove. +- optional `--config`, `--campaign`, and `--session` override default config discovery. +- list mode prints effective locks from static `pipeline.archive.locks` and remote `{session_prefix}/locks.yml`. +- `locks add` writes only the remote lock store and fails if the source is already locked by pipeline config. +- `locks remove` removes only remote locks and cannot remove static pipeline locks. +- `locks add --force` is required to update an existing remote lock reason. + +Examples: + +```bash +narratio locks --session-id 2026-04-04 +narratio locks add --session-id 2026-04-04 --reason "manual transcript review" narratio.transcript.trimmed +narratio locks remove --session-id 2026-04-04 narratio.transcript.trimmed +``` ### `run-stage` diff --git a/docs/config.md b/docs/config.md index 1951bef..6185ec6 100644 --- a/docs/config.md +++ b/docs/config.md @@ -394,7 +394,7 @@ Remote mutable lock store: - path: `{root_prefix}/campaigns/{campaign}/sessions/{session_id}/locks.yml`. - strict YAML shape: top-level `locks`, each with `source` and optional `reason`. -- `narratio lock` and `narratio unlock` mutate only the remote lock store. +- `narratio locks add` and `narratio locks remove` mutate only the remote lock store. - writes use existence checks plus `--force` for updates; they are not compare-and-swap atomic. Restore-related implications: diff --git a/docs/operations.md b/docs/operations.md index 67b6328..2bbff1e 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -153,10 +153,10 @@ Archive promotion is explicit and source-based: Lock helper behavior: - `narratio locks --session-id ` lists effective static and remote locks. -- `narratio lock --session-id --reason ` writes or updates a remote lock. -- `narratio unlock --session-id ` removes only a remote lock. -- `lock --force` is required to update an existing remote lock reason. -- `unlock` cannot remove static pipeline locks. +- `narratio locks add --session-id --reason ` writes a remote lock. +- `narratio locks add --session-id --force --reason ` updates an existing remote lock reason. +- `narratio locks remove --session-id ` removes only a remote lock. +- `locks remove` cannot remove static pipeline locks. - remote lock writes check whether the lock store exists, but are not compare-and-swap atomic. ## Resume, retry, restore, and safe rerun behavior diff --git a/docs/roadmap/operations.md b/docs/roadmap/operations.md index 14a13b6..c86e06e 100644 --- a/docs/roadmap/operations.md +++ b/docs/roadmap/operations.md @@ -22,22 +22,22 @@ The operator helper command set is no longer conceptual. Current behavior is doc - `narratio artifacts list` - `narratio artifacts list --remote` - `narratio locks` -- `narratio lock ` -- `narratio unlock ` +- `narratio locks add ` +- `narratio locks remove ` ## Implemented Decisions - Helper output is text-only. No JSON schema exists yet. - `status` remains a top-level command. - `session validate`, `session init`, and `artifacts list` are nested helper commands. -- `lock`, `unlock`, and `locks` are top-level commands. +- `locks` is the single top-level command for listing, adding, and removing archive promotion locks. - Remote session initialization requires explicit `--remote`. - Local session initialization requires `--output`. - Remote artifact availability is opt-in with `artifacts list --remote`. - Mutable locks are source-based and stored at `{session_prefix}/locks.yml`. - The remote lock store uses strict YAML with top-level `locks`. - Static `pipeline.archive.locks` and remote locks are merged; static locks win on duplicate sources. -- `unlock` removes only remote locks. +- `locks remove` removes only remote locks. - Ordinary execution `--force` does not override locks. - Remote lock writes use existence checks and `--force` for updates; there is no compare-and-swap protection. @@ -50,4 +50,3 @@ These are intentionally not implemented: - Rich remote artifact availability across historical run-local objects. - Session-lock acquisition for remote mutation helpers. - Broader campaign helper commands such as `campaign validate` or `campaign publish`. - diff --git a/internal/app/commands.go b/internal/app/commands.go index 1b2a9a5..a5a5163 100644 --- a/internal/app/commands.go +++ b/internal/app/commands.go @@ -7,7 +7,7 @@ import ( "strings" ) -var supportedCommands = []string{"run", "plan", "status", "resume", "run-stage", "restore", "session", "artifacts", "locks", "lock", "unlock"} +var supportedCommands = []string{"run", "plan", "status", "resume", "run-stage", "restore", "session", "artifacts", "locks"} // Execute dispatches CLI commands and returns a process exit code. func Execute(args []string, stdout, stderr io.Writer) int { @@ -40,10 +40,6 @@ func Execute(args []string, stdout, stderr io.Writer) int { err = Artifacts(ctx, cmdArgs, stdout) case "locks": err = Locks(ctx, cmdArgs, stdout) - case "lock": - err = Lock(ctx, cmdArgs, stdout) - case "unlock": - err = Unlock(ctx, cmdArgs, stdout) default: fmt.Fprintf(stderr, "unknown command: %q\n\n", cmd) printUsage(stderr) diff --git a/internal/app/operator_helpers.go b/internal/app/operator_helpers.go index 879e213..14aa8a6 100644 --- a/internal/app/operator_helpers.go +++ b/internal/app/operator_helpers.go @@ -379,8 +379,23 @@ func ArtifactsList(ctx context.Context, args []string, out io.Writer) error { return nil } -// Locks lists effective archive locks. +// Locks dispatches archive lock list and mutation helpers. func Locks(ctx context.Context, args []string, out io.Writer) error { + if len(args) > 0 && !strings.HasPrefix(args[0], "-") { + switch args[0] { + case "add": + return LocksAdd(ctx, args[1:], out) + case "remove": + return LocksRemove(ctx, args[1:], out) + default: + return fmt.Errorf("locks: unknown subcommand %q", args[0]) + } + } + return LocksList(ctx, args, out) +} + +// LocksList lists effective archive locks. +func LocksList(ctx context.Context, args []string, out io.Writer) error { fs := flag.NewFlagSet("locks", flag.ContinueOnError) fs.SetOutput(io.Discard) var flags commonConfigFlags @@ -391,6 +406,9 @@ func Locks(ctx context.Context, args []string, out io.Writer) error { if fs.NArg() != 0 { return fmt.Errorf("locks: unexpected positional arguments") } + if strings.TrimSpace(flags.sessionID) == "" { + return fmt.Errorf("locks: --session-id is required") + } cfg, _, locks, _, err := loadHelperContext(ctx, flags, true) if err != nil { return fmt.Errorf("locks: %w", err) @@ -399,9 +417,9 @@ func Locks(ctx context.Context, args []string, out io.Writer) error { return nil } -// Lock adds or updates one remote lock. -func Lock(ctx context.Context, args []string, out io.Writer) error { - fs := flag.NewFlagSet("lock", flag.ContinueOnError) +// LocksAdd adds or updates one remote lock. +func LocksAdd(ctx context.Context, args []string, out io.Writer) error { + fs := flag.NewFlagSet("locks add", flag.ContinueOnError) fs.SetOutput(io.Discard) var flags commonConfigFlags var reason string @@ -410,71 +428,77 @@ func Lock(ctx context.Context, args []string, out io.Writer) error { fs.StringVar(&reason, "reason", "", "lock reason") fs.BoolVar(&force, "force", false, "update existing remote lock") if err := fs.Parse(args); err != nil { - return fmt.Errorf("lock: invalid flags: %w", err) + return fmt.Errorf("locks add: invalid flags: %w", err) } if fs.NArg() != 1 { - return fmt.Errorf("lock: expected exactly one source id") + return fmt.Errorf("locks add: expected exactly one source id") + } + if strings.TrimSpace(flags.sessionID) == "" { + return fmt.Errorf("locks add: --session-id is required") } source := strings.TrimSpace(fs.Arg(0)) cfg, store, locks, _, err := loadHelperContext(ctx, flags, true) if err != nil { - return fmt.Errorf("lock: %w", err) + return fmt.Errorf("locks add: %w", err) } - if _, err := config.ValidateArchiveLockRules([]config.ArchiveLockRule{{Source: source}}, cfg.Pipeline.Scriptorium, "lock"); err != nil { - return fmt.Errorf("lock: %w", err) + if _, err := config.ValidateArchiveLockRules([]config.ArchiveLockRule{{Source: source}}, cfg.Pipeline.Scriptorium, "locks add"); err != nil { + return fmt.Errorf("locks add: %w", err) } if _, ok := lockSourceSet(locks.Static)[source]; ok { - return fmt.Errorf("lock: source %q is locked by pipeline config and cannot be modified remotely", source) + return fmt.Errorf("locks add: source %q is locked by pipeline config and cannot be modified remotely", source) } remoteSet := lockSourceSet(locks.Remote) if _, exists := remoteSet[source]; exists && !force { - return fmt.Errorf("lock: remote lock for %q already exists; pass --force to update", source) + return fmt.Errorf("locks add: remote lock for %q already exists; pass --force to update", source) } remoteSet[source] = config.ArchiveLockRule{Source: source, Reason: strings.TrimSpace(reason)} remoteLocks := lockMapValues(remoteSet) if _, err := config.ValidateArchiveLockRules(remoteLocks, cfg.Pipeline.Scriptorium, "locks"); err != nil { - return fmt.Errorf("lock: %w", err) + return fmt.Errorf("locks add: %w", err) } if err := uploadRemoteLockStore(ctx, store, locks.Key, &config.ArchiveLockStore{Locks: remoteLocks}); err != nil { - return fmt.Errorf("lock: %w", err) + return fmt.Errorf("locks add: %w", err) } - _, err = fmt.Fprintf(out, "narratio lock: locked %s\n", source) + _, err = fmt.Fprintf(out, "narratio locks add: locked %s\n", source) return err } -// Unlock removes one remote lock. -func Unlock(ctx context.Context, args []string, out io.Writer) error { - fs := flag.NewFlagSet("unlock", flag.ContinueOnError) +// LocksRemove removes one remote lock. +func LocksRemove(ctx context.Context, args []string, out io.Writer) error { + fs := flag.NewFlagSet("locks remove", flag.ContinueOnError) fs.SetOutput(io.Discard) var flags commonConfigFlags addCommonConfigFlags(fs, &flags) if err := fs.Parse(args); err != nil { - return fmt.Errorf("unlock: invalid flags: %w", err) + return fmt.Errorf("locks remove: invalid flags: %w", err) } if fs.NArg() != 1 { - return fmt.Errorf("unlock: expected exactly one source id") + return fmt.Errorf("locks remove: expected exactly one source id") + } + if strings.TrimSpace(flags.sessionID) == "" { + return fmt.Errorf("locks remove: --session-id is required") } source := strings.TrimSpace(fs.Arg(0)) cfg, store, locks, _, err := loadHelperContext(ctx, flags, true) if err != nil { - return fmt.Errorf("unlock: %w", err) + return fmt.Errorf("locks remove: %w", err) } - if _, err := config.ValidateArchiveLockRules([]config.ArchiveLockRule{{Source: source}}, cfg.Pipeline.Scriptorium, "unlock"); err != nil { - return fmt.Errorf("unlock: %w", err) + if _, err := config.ValidateArchiveLockRules([]config.ArchiveLockRule{{Source: source}}, cfg.Pipeline.Scriptorium, "locks remove"); err != nil { + return fmt.Errorf("locks remove: %w", err) } remoteSet := lockSourceSet(locks.Remote) if _, ok := remoteSet[source]; !ok { if _, static := lockSourceSet(locks.Static)[source]; static { - return fmt.Errorf("unlock: source %q is locked by pipeline config and cannot be unlocked remotely", source) + return fmt.Errorf("locks remove: source %q is locked by pipeline config and cannot be unlocked remotely", source) } - return fmt.Errorf("unlock: remote lock for %q does not exist", source) + return fmt.Errorf("locks remove: remote lock for %q does not exist", source) } delete(remoteSet, source) remoteLocks := lockMapValues(remoteSet) if err := uploadRemoteLockStore(ctx, store, locks.Key, &config.ArchiveLockStore{Locks: remoteLocks}); err != nil { - return fmt.Errorf("unlock: %w", err) + return fmt.Errorf("locks remove: %w", err) } - _, err = fmt.Fprintf(out, "narratio unlock: unlocked %s\n", source) + _, err = fmt.Fprintf(out, "narratio locks remove: unlocked %s\n", source) return err } diff --git a/internal/app/operator_helpers_test.go b/internal/app/operator_helpers_test.go index 6b8087d..22b7c54 100644 --- a/internal/app/operator_helpers_test.go +++ b/internal/app/operator_helpers_test.go @@ -91,7 +91,7 @@ inputs: } } -func TestExecuteLockAndUnlockUseRemoteLockStore(t *testing.T) { +func TestExecuteLocksAddListAndRemoveUseRemoteLockStore(t *testing.T) { workspaceRoot := t.TempDir() pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) fake := &storage.FakeBackend{} @@ -101,15 +101,16 @@ func TestExecuteLockAndUnlockUseRemoteLockStore(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer code := Execute([]string{ - "lock", + "locks", "add", "--config", pipelinePath, "--campaign", campaignPath, "--session", sessionPath, + "--session-id", "2026-05-03", "--reason", "manual edit", "narratio.transcript.trimmed", }, &stdout, &stderr) if code != 0 { - t.Fatalf("lock exit code = %d, want 0; stderr=%q", code, stderr.String()) + t.Fatalf("locks add exit code = %d, want 0; stderr=%q", code, stderr.String()) } key := artifacts.S3SessionLocksKey(artifacts.S3SessionPrefix("dnd", "sample-campaign", "2026-05-03")) obj, ok := fake.Objects[key] @@ -123,24 +124,187 @@ func TestExecuteLockAndUnlockUseRemoteLockStore(t *testing.T) { stdout.Reset() stderr.Reset() code = Execute([]string{ - "unlock", + "locks", "--config", pipelinePath, "--campaign", campaignPath, "--session", sessionPath, + "--session-id", "2026-05-03", + }, &stdout, &stderr) + if code != 0 { + t.Fatalf("locks list exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "- narratio.transcript.trimmed origin=remote") { + t.Fatalf("stdout = %q, want remote lock", stdout.String()) + } + + stdout.Reset() + stderr.Reset() + code = Execute([]string{ + "locks", "remove", + "--config", pipelinePath, + "--campaign", campaignPath, + "--session", sessionPath, + "--session-id", "2026-05-03", "narratio.transcript.trimmed", }, &stdout, &stderr) if code != 0 { - t.Fatalf("unlock exit code = %d, want 0; stderr=%q", code, stderr.String()) + t.Fatalf("locks remove exit code = %d, want 0; stderr=%q", code, stderr.String()) } store, err := config.LoadArchiveLockStoreBytes("locks.yml", fake.Objects[key].Data, nil) if err != nil { t.Fatalf("LoadArchiveLockStoreBytes() error = %v", err) } if len(store.Locks) != 0 { - t.Fatalf("locks after unlock = %#v, want empty", store.Locks) + t.Fatalf("locks after remove = %#v, want empty", store.Locks) } - if storeInitCalls != 2 { - t.Fatalf("object store init calls = %d, want 2", storeInitCalls) + if storeInitCalls != 3 { + t.Fatalf("object store init calls = %d, want 3", storeInitCalls) + } +} + +func TestExecuteLocksAddDuplicateRequiresForce(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) + fake := &storage.FakeBackend{} + var storeInitCalls int + restoreAppConfigTestGlobals(t, fake, &storeInitCalls, []string{sessionPath}) + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{ + "locks", "add", + "--config", pipelinePath, + "--campaign", campaignPath, + "--session", sessionPath, + "--session-id", "2026-05-03", + "--reason", "first", + "narratio.transcript.trimmed", + }, &stdout, &stderr) + if code != 0 { + t.Fatalf("initial locks add exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + + stdout.Reset() + stderr.Reset() + code = Execute([]string{ + "locks", "add", + "--config", pipelinePath, + "--campaign", campaignPath, + "--session", sessionPath, + "--session-id", "2026-05-03", + "--reason", "second", + "narratio.transcript.trimmed", + }, &stdout, &stderr) + if code == 0 { + t.Fatal("duplicate locks add exit code = 0, want non-zero") + } + if !strings.Contains(stderr.String(), "pass --force to update") { + t.Fatalf("stderr = %q, want force guidance", stderr.String()) + } + + stdout.Reset() + stderr.Reset() + code = Execute([]string{ + "locks", "add", + "--config", pipelinePath, + "--campaign", campaignPath, + "--session", sessionPath, + "--session-id", "2026-05-03", + "--reason", "second", + "--force", + "narratio.transcript.trimmed", + }, &stdout, &stderr) + if code != 0 { + t.Fatalf("forced locks add exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + key := artifacts.S3SessionLocksKey(artifacts.S3SessionPrefix("dnd", "sample-campaign", "2026-05-03")) + if !strings.Contains(string(fake.Objects[key].Data), "reason: second") { + t.Fatalf("lock store data = %q, want updated reason", string(fake.Objects[key].Data)) + } +} + +func TestExecuteLocksRequireSessionID(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {"list", []string{"locks"}, "locks: --session-id is required"}, + {"add", []string{"locks", "add", "narratio.transcript.trimmed"}, "locks add: --session-id is required"}, + {"remove", []string{"locks", "remove", "narratio.transcript.trimmed"}, "locks remove: --session-id is required"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute(tt.args, &stdout, &stderr) + if code == 0 { + t.Fatal("exit code = 0, want non-zero") + } + if !strings.Contains(stderr.String(), tt.want) { + t.Fatalf("stderr = %q, want %q", stderr.String(), tt.want) + } + }) + } +} + +func TestExecuteLocksCannotModifyStaticLocks(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) + addStaticArchiveLockToPipelineConfig(t, pipelinePath, "narratio.transcript.trimmed") + fake := &storage.FakeBackend{} + var storeInitCalls int + restoreAppConfigTestGlobals(t, fake, &storeInitCalls, []string{sessionPath}) + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{ + "locks", "add", + "--config", pipelinePath, + "--campaign", campaignPath, + "--session", sessionPath, + "--session-id", "2026-05-03", + "narratio.transcript.trimmed", + }, &stdout, &stderr) + if code == 0 { + t.Fatal("locks add static lock exit code = 0, want non-zero") + } + if !strings.Contains(stderr.String(), "locked by pipeline config") { + t.Fatalf("stderr = %q, want static lock error", stderr.String()) + } + + stdout.Reset() + stderr.Reset() + code = Execute([]string{ + "locks", "remove", + "--config", pipelinePath, + "--campaign", campaignPath, + "--session", sessionPath, + "--session-id", "2026-05-03", + "narratio.transcript.trimmed", + }, &stdout, &stderr) + if code == 0 { + t.Fatal("locks remove static lock exit code = 0, want non-zero") + } + if !strings.Contains(stderr.String(), "locked by pipeline config") { + t.Fatalf("stderr = %q, want static lock error", stderr.String()) + } +} + +func TestExecuteTopLevelLockAndUnlockAreRemoved(t *testing.T) { + tests := []string{"lock", "unlock"} + for _, cmd := range tests { + t.Run(cmd, func(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{cmd, "narratio.transcript.trimmed"}, &stdout, &stderr) + if code == 0 { + t.Fatal("exit code = 0, want non-zero") + } + if !strings.Contains(stderr.String(), `unknown command: "`+cmd+`"`) { + t.Fatalf("stderr = %q, want unknown command", stderr.String()) + } + }) } } @@ -237,3 +401,23 @@ func writeValidArchiveConfigFiles(t *testing.T, workspaceRoot string) (string, s } return pipelinePath, campaignPath, sessionPath } + +func addStaticArchiveLockToPipelineConfig(t *testing.T, pipelinePath, source string) { + t.Helper() + data, err := os.ReadFile(pipelinePath) + if err != nil { + t.Fatalf("read pipeline: %v", err) + } + updated := strings.Replace( + string(data), + "archive:\n enabled: true\n upload_run: false\n", + "archive:\n enabled: true\n upload_run: false\n locks:\n - source: "+source+"\n reason: static review\n", + 1, + ) + if updated == string(data) { + t.Fatalf("archive section not found in pipeline config") + } + if err := os.WriteFile(pipelinePath, []byte(updated), 0o644); err != nil { + t.Fatalf("write pipeline: %v", err) + } +}