Skip to content

Commit 10052c0

Browse files
authored
feat(root): --non-interactive persistent flag for jtk + cfl (#391) (#394)
* feat(root): add --non-interactive persistent flag for jtk + cfl Add --non-interactive as a persistent root flag on both tools per cli-common scriptability.md §3.4/§4.1. Under it: - Init wizards (jtk init / cfl init) skip the huh form entirely and fail loud naming the first missing required field (URL, email or cloud-id, token). cfl's error points to `cfl set-credential` since cfl init has no --token flag. - The sibling-impact confirm proceeds (the user opted in by passing --non-interactive on the shared store) with an info line to stderr for the audit trail. The legacy-cleanup confirm skips both prompt and deletion (non-destructive default). - All 11 destructive-confirm sites (jtk issues/projects/automation/ fields*3/configcmd, cfl page/space/attachment/configcmd) adopt prompt.ConfirmOrFail(force, nonInteractive, stdin); without --force they return prompt.ErrConfirmationRequired ("re-run with --force to proceed"). The prompt-text emission is also gated on !NonInteractive so CI logs stay clean — stderr is empty too. - The file-backend passphrase callback consults a package-level GetNonInteractive() and fails loud asking for ATLASSIAN_CLI_KEYRING_PASSPHRASE rather than prompting on a real TTY. Wiring threads through WireBackendSelection (the single chokepoint shadowing PersistentPreRunE subcommands already remember to call) so dashboards/boards/automation/sprints get the gate automatically. A regression test pins this coupling. Closes #391 * fix(non-interactive): short-circuit destructive paths before API/keyring inspection Address Codex PR #394 majors: - All destructive paths that previously fetched a remote resource or opened the keyring before checking the confirmation gate now short-circuit early: `if NonInteractive && !force { return prompt.ErrConfirmationRequired }`. Affected: cfl page/space/attachment delete, jtk automation delete, and both config clear commands. Under --non-interactive without --force the user now gets the confirmation error verbatim, never an API/auth/not-found or keyring/passphrase error winning first. - The two config-clear commands also gain the early short-circuit so the destructive warning block (which prints to stderr unconditionally before the `confirm("Proceed?")` call) cannot leak under --non-interactive. CI logs see neither warning nor `[y/N]` prompt before the fail-loud error. - passphrase_test.go no longer hangs when run from a real TTY: skips the non-TTY-fallback path under term.IsTerminal(os.Stdin) since the callback would block in term.ReadPassword. The --non-interactive branch still covers fail-loud independently. - Add destructive-confirm regression coverage at the API-lookup-before -confirm boundary (cfl page delete) and the config-clear boundary (cfl configcmd clear) — both assert ErrConfirmationRequired surfaces, the API isn't hit / the keyring isn't inspected, and stderr stays empty. The pattern is identical across jtk/cfl resource delete commands and across both config-clear paths. * test(non-interactive): close TDD-pass coverage gaps Address the TDD-pass majors by pinning the §3.4 contract at sites that weren't previously asserted: - cfl wireBackendSelection NonInteractive threading + default-false (mirrors the existing jtk test that pins this single chokepoint). - cfl attachment delete: API not hit + stderr empty under --non-interactive without --force; ErrConfirmationRequired surfaces. - jtk automation delete: same short-circuit assertion + API-miss check. - jtk configcmd clear: short-circuit + stderr-empty + token-preserved assertions. The pattern is identical at each call-site; pinning the four most critical destructive paths (each of which involved a pre-confirm API or keyring fetch the short-circuit must precede) makes a regression loud across the family. * fix+test(non-interactive): close daemon-round-1 coverage + positioning gaps - cfl space delete: move the --non-interactive short-circuit before view.ValidateFormat so a bogus --output value can't win precedence over the §3.4 confirmation contract. New delete_test pins both the short-circuit AND the format-validation precedence. - jtk automation delete: defense-in-depth — guard the warning-text block on `!force && !opts.NonInteractive` to match issues/page/ attachment so a future refactor that moves the early short-circuit can't leak warning text under --non-interactive. Add WithForce proceeds test + stdout-empty assertion to the existing short-circuit test. - jtk projects delete: add WithoutForce_FailsLoud + WithForce_Proceeds tests (mirrors the issues pattern). - jtk fields delete×3 (runDelete + runContextsDelete + runOptionsDelete): single table-driven test covers all three sites' §3.4 contract. - jtk configcmd clear: add WithForce_Proceeds test (mirrors cfl).
1 parent 58742b0 commit 10052c0

34 files changed

Lines changed: 1418 additions & 178 deletions

CLAUDE.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,21 @@ backend only (never the value). `config clear` removes the single shared
204204
resolve the same key); `config clear --all` removes the whole bundle
205205
(including any deprecated per-tool keys) plus the non-secret config file.
206206

207+
**`--non-interactive` persistent root flag (§3.4):** both jtk and cfl
208+
accept `--non-interactive` on every command. Under it: no interactive
209+
prompts run (init wizards, destructive-confirm questions, file-backend
210+
passphrase TTY prompt). Init wizards fail loud naming the first missing
211+
required field (URL, email/cloud-id, token). Destructive commands
212+
(`issues/projects/automation/fields/config clear` on jtk;
213+
`page/space/attachment/config clear` on cfl) require `--force` to
214+
proceed; without it they return a clear "confirmation required;
215+
re-run with --force" error. Init's sibling-impact confirm and
216+
legacy-cleanup confirms default to safe behavior under
217+
`--non-interactive` (proceed-with-save, leave-legacy-files-in-place).
218+
The flag is forwarded into the keyring package via `WireBackendSelection`
219+
so the file-backend passphrase callback fails loud asking for
220+
`ATLASSIAN_CLI_KEYRING_PASSPHRASE` rather than prompting on a real TTY.
221+
207222
## Git History
208223

209224
This monorepo was created using `git subtree` to preserve the full commit history of both tools. Use `git log --oneline` to see the complete history from both source repositories.

shared/keyring/non_interactive.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package keyring
2+
3+
import "sync"
4+
5+
// SetNonInteractive wires the root-command --non-interactive policy into
6+
// the package state consulted by the file-backend passphrase callback.
7+
//
8+
// Mirrors the SetBackendSelection pattern at keyring.go:28. Callers
9+
// (each tool's root PersistentPreRunE) invoke this once before any
10+
// Open* runs; the mutex guards against surprise concurrent callers
11+
// (test code rebuilding the command tree, future goroutine-launching
12+
// subcommands).
13+
func SetNonInteractive(nonInteractive bool) {
14+
nonInteractiveMu.Lock()
15+
defer nonInteractiveMu.Unlock()
16+
selectedNonInteractive = nonInteractive
17+
}
18+
19+
// GetNonInteractive returns the current package-level non-interactive
20+
// state. Consumed by passphraseFunc to fail loud on --non-interactive
21+
// even when stdin is a real TTY.
22+
func GetNonInteractive() bool {
23+
nonInteractiveMu.RLock()
24+
defer nonInteractiveMu.RUnlock()
25+
return selectedNonInteractive
26+
}
27+
28+
var (
29+
nonInteractiveMu sync.RWMutex
30+
selectedNonInteractive bool
31+
)

shared/keyring/passphrase.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,15 @@ func passphraseFunc(service string) func() (string, error) {
3131
// must supply ATLASSIAN_CLI_KEYRING_PASSPHRASE in that case (the
3232
// error message says so). Token delivery and passphrase entry
3333
// cannot share stdin.
34+
//
35+
// Additionally, the --non-interactive root flag (§3.4) forces
36+
// fail-loud even on a real TTY: a scripted/CI run must never
37+
// block on a prompt, regardless of where it is invoked from.
38+
if GetNonInteractive() {
39+
return "", fmt.Errorf(
40+
"file keyring backend needs a passphrase: set %s (--non-interactive disables the TTY prompt fallback)",
41+
passphraseEnvVar(service))
42+
}
3443
if !term.IsTerminal(int(os.Stdin.Fd())) {
3544
return "", fmt.Errorf(
3645
"file keyring backend needs a passphrase: set %s, or run interactively",

shared/keyring/passphrase_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package keyring
2+
3+
import (
4+
"os"
5+
"strings"
6+
"testing"
7+
8+
"golang.org/x/term"
9+
)
10+
11+
// TestPassphraseFunc_NonInteractiveFailsLoud — under --non-interactive
12+
// the file-backend passphrase callback MUST fail loud asking for the
13+
// env var, regardless of whether stdin is a real TTY. The error message
14+
// must NOT include the "or run interactively" hint that the non-TTY
15+
// path uses, since the user explicitly opted out of interactive mode.
16+
func TestPassphraseFunc_NonInteractiveFailsLoud(t *testing.T) {
17+
SetNonInteractive(true)
18+
defer SetNonInteractive(false)
19+
20+
fn := passphraseFunc("atlassian-cli")
21+
got, err := fn()
22+
if err == nil {
23+
t.Fatalf("expected error, got passphrase %q", got)
24+
}
25+
if got != "" {
26+
t.Fatalf("passphrase must be empty on failure, got %q", got)
27+
}
28+
if !strings.Contains(err.Error(), "ATLASSIAN_CLI_KEYRING_PASSPHRASE") {
29+
t.Fatalf("error must name the env var, got %v", err)
30+
}
31+
if !strings.Contains(err.Error(), "--non-interactive") {
32+
t.Fatalf("error must explain the --non-interactive policy, got %v", err)
33+
}
34+
if strings.Contains(err.Error(), "or run interactively") {
35+
t.Fatalf("under --non-interactive the 'or run interactively' hint is wrong, got %v", err)
36+
}
37+
}
38+
39+
// TestPassphraseFunc_NonInteractiveOff_NonTTYPath — the non-TTY-stdin
40+
// path (the pre-existing contract) keeps working when --non-interactive
41+
// is NOT set. Skip when os.Stdin IS a real TTY (running tests in a
42+
// terminal) because the callback would enter term.ReadPassword and
43+
// block. The non-interactive-true branch above covers fail-loud; the
44+
// interactive prompt branch requires a PTY harness we don't have here.
45+
func TestPassphraseFunc_NonInteractiveOff_NonTTYPath(t *testing.T) {
46+
SetNonInteractive(false)
47+
if term.IsTerminal(int(os.Stdin.Fd())) {
48+
t.Skip("os.Stdin is a real TTY; the prompt path would block — skipping (covered by the --non-interactive=true branch)")
49+
}
50+
fn := passphraseFunc("atlassian-cli")
51+
got, err := fn()
52+
if err == nil {
53+
t.Fatalf("expected non-TTY-fallback error, got passphrase %q", got)
54+
}
55+
if got != "" {
56+
t.Fatalf("passphrase must be empty on failure, got %q", got)
57+
}
58+
if !strings.Contains(err.Error(), "or run interactively") {
59+
t.Fatalf("non-TTY fallback error must mention 'or run interactively', got %v", err)
60+
}
61+
}

shared/prompt/confirm.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,12 @@ package prompt
33

44
import (
55
"bufio"
6+
"errors"
67
"io"
8+
"os"
79
"strings"
10+
11+
"golang.org/x/term"
812
)
913

1014
// Confirm prompts the user for confirmation and returns true if they answer yes.
@@ -30,3 +34,38 @@ func ConfirmOrForce(force bool, stdin io.Reader) (bool, error) {
3034
}
3135
return Confirm(stdin)
3236
}
37+
38+
// ErrConfirmationRequired is the §3.4 sentinel: destructive operations
39+
// under --non-interactive without --force MUST fail loud rather than
40+
// block on stdin or silently cancel. The text is the actionable hint.
41+
var ErrConfirmationRequired = errors.New("--non-interactive: confirmation required; re-run with --force to proceed")
42+
43+
// ConfirmOrFail upgrades ConfirmOrForce with the §3.4 non-interactive
44+
// gate. Precedence: --force wins (returns true); --non-interactive
45+
// without --force returns ErrConfirmationRequired; otherwise prompts
46+
// via Confirm.
47+
func ConfirmOrFail(force, nonInteractive bool, stdin io.Reader) (bool, error) {
48+
if force {
49+
return true, nil
50+
}
51+
if nonInteractive {
52+
return false, ErrConfirmationRequired
53+
}
54+
return Confirm(stdin)
55+
}
56+
57+
// WantPrompt reports whether interactive prompts should run. Returns
58+
// false under --non-interactive (regardless of stdin) AND when stdin is
59+
// not a TTY. Used by init wizards to choose between a huh form (TTY +
60+
// interactive) and a fail-loud validator (non-interactive or non-TTY).
61+
func WantPrompt(nonInteractive bool, stdin io.Reader) bool {
62+
if nonInteractive {
63+
return false
64+
}
65+
return isTerminal(stdin)
66+
}
67+
68+
func isTerminal(r io.Reader) bool {
69+
f, ok := r.(*os.File)
70+
return ok && term.IsTerminal(int(f.Fd()))
71+
}

shared/prompt/confirm_test.go

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package prompt
22

33
import (
4+
"errors"
5+
"os"
46
"strings"
57
"testing"
68

@@ -114,3 +116,78 @@ func TestConfirmOrForce(t *testing.T) {
114116
})
115117
}
116118
}
119+
120+
func TestConfirmOrFail(t *testing.T) {
121+
t.Parallel()
122+
tests := []struct {
123+
name string
124+
force bool
125+
nonInteractive bool
126+
input string
127+
want bool
128+
wantErr error
129+
}{
130+
{
131+
name: "force wins over non-interactive",
132+
force: true,
133+
nonInteractive: true,
134+
want: true,
135+
},
136+
{
137+
name: "non-interactive without force surfaces sentinel",
138+
nonInteractive: true,
139+
wantErr: ErrConfirmationRequired,
140+
},
141+
{
142+
name: "interactive y confirms",
143+
input: "y\n",
144+
want: true,
145+
},
146+
{
147+
name: "interactive n cancels",
148+
input: "n\n",
149+
want: false,
150+
},
151+
}
152+
for _, tt := range tests {
153+
t.Run(tt.name, func(t *testing.T) {
154+
t.Parallel()
155+
got, err := ConfirmOrFail(tt.force, tt.nonInteractive, strings.NewReader(tt.input))
156+
if tt.wantErr != nil {
157+
if !errors.Is(err, tt.wantErr) {
158+
t.Fatalf("want %v, got %v", tt.wantErr, err)
159+
}
160+
return
161+
}
162+
testutil.RequireNoError(t, err)
163+
testutil.Equal(t, got, tt.want)
164+
})
165+
}
166+
}
167+
168+
func TestWantPrompt(t *testing.T) {
169+
t.Parallel()
170+
171+
// Non-TTY reader (strings.Reader isn't *os.File so isTerminal returns false).
172+
nonTTY := strings.NewReader("")
173+
174+
if WantPrompt(true, nonTTY) {
175+
t.Fatal("nonInteractive=true must force WantPrompt=false")
176+
}
177+
if WantPrompt(false, nonTTY) {
178+
t.Fatal("non-TTY stdin must yield WantPrompt=false")
179+
}
180+
181+
// Build a real TTY-like *os.File via os.Pipe(); the read end isn't a
182+
// terminal so isTerminal returns false. We can't easily fabricate a
183+
// real TTY in a unit test, so assert the false-case from a real *os.File
184+
// to cover that branch of the type assertion.
185+
r, w, err := os.Pipe()
186+
if err != nil {
187+
t.Fatalf("os.Pipe: %v", err)
188+
}
189+
defer func() { _ = r.Close(); _ = w.Close() }()
190+
if WantPrompt(false, r) {
191+
t.Fatal("pipe read end is not a TTY; WantPrompt must be false")
192+
}
193+
}

tools/cfl/internal/cmd/attachment/delete.go

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
package attachment
22

33
import (
4-
"bufio"
54
"context"
65
"fmt"
76

87
"github.com/spf13/cobra"
98

9+
"github.com/open-cli-collective/atlassian-go/prompt"
10+
1011
"github.com/open-cli-collective/confluence-cli/internal/cmd/root"
1112
)
1213

@@ -39,6 +40,13 @@ func newDeleteCmd(rootOpts *root.Options) *cobra.Command {
3940
}
4041

4142
func runDeleteAttachment(ctx context.Context, attachmentID string, opts *deleteOptions) error {
43+
// §3.4: short-circuit BEFORE any API call so --non-interactive without
44+
// --force returns ErrConfirmationRequired even if the attachment
45+
// lookup would have failed first (auth/not-found/network).
46+
if opts.NonInteractive && !opts.force {
47+
return prompt.ErrConfirmationRequired
48+
}
49+
4250
client, err := opts.APIClient()
4351
if err != nil {
4452
return err
@@ -51,20 +59,17 @@ func runDeleteAttachment(ctx context.Context, attachmentID string, opts *deleteO
5159

5260
v := opts.View()
5361

54-
if !opts.force {
62+
if !opts.force && !opts.NonInteractive {
5563
_, _ = fmt.Fprintf(opts.Stderr, "About to delete attachment: %s (ID: %s)\n", attachment.Title, attachment.ID)
5664
_, _ = fmt.Fprint(opts.Stderr, "Are you sure? [y/N]: ")
57-
58-
scanner := bufio.NewScanner(opts.Stdin)
59-
var confirm string
60-
if scanner.Scan() {
61-
confirm = scanner.Text()
62-
}
63-
64-
if confirm != "y" && confirm != "Y" {
65-
_, _ = fmt.Fprintln(opts.Stderr, "Deletion cancelled.")
66-
return nil
67-
}
65+
}
66+
confirmed, err := prompt.ConfirmOrFail(opts.force, opts.NonInteractive, opts.Stdin)
67+
if err != nil {
68+
return err
69+
}
70+
if !confirmed {
71+
_, _ = fmt.Fprintln(opts.Stderr, "Deletion cancelled.")
72+
return nil
6873
}
6974

7075
if err := client.DeleteAttachment(ctx, attachmentID); err != nil {

tools/cfl/internal/cmd/attachment/delete_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,13 @@ package attachment
33
import (
44
"bytes"
55
"context"
6+
"errors"
67
"net/http"
78
"net/http/httptest"
89
"strings"
910
"testing"
1011

12+
"github.com/open-cli-collective/atlassian-go/prompt"
1113
"github.com/open-cli-collective/atlassian-go/testutil"
1214

1315
"github.com/open-cli-collective/confluence-cli/api"
@@ -240,3 +242,36 @@ func TestRunDeleteAttachment_DeleteFails(t *testing.T) {
240242
testutil.RequireError(t, err)
241243
testutil.Contains(t, err.Error(), "deleting attachment")
242244
}
245+
246+
// TestRunDeleteAttachment_NonInteractive_WithoutForce_ShortCircuits
247+
// pins the §3.4 early-fail contract: the API must NOT be called when
248+
// --non-interactive is set without --force.
249+
func TestRunDeleteAttachment_NonInteractive_WithoutForce_ShortCircuits(t *testing.T) {
250+
t.Parallel()
251+
var hits int
252+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
253+
hits++
254+
w.WriteHeader(http.StatusOK)
255+
}))
256+
defer server.Close()
257+
258+
rootOpts := newTestRootOptions()
259+
rootOpts.NonInteractive = true
260+
client := api.NewClient(server.URL, "test@example.com", "token")
261+
rootOpts.SetAPIClient(client)
262+
263+
opts := &deleteOptions{Options: rootOpts, force: false}
264+
err := runDeleteAttachment(context.Background(), "att123", opts)
265+
if err == nil {
266+
t.Fatal("expected ErrConfirmationRequired")
267+
}
268+
if !errors.Is(err, prompt.ErrConfirmationRequired) {
269+
t.Fatalf("expected prompt.ErrConfirmationRequired, got %v", err)
270+
}
271+
if hits != 0 {
272+
t.Fatalf("API must not be called; got %d hits", hits)
273+
}
274+
if rootOpts.Stderr.(*bytes.Buffer).Len() != 0 {
275+
t.Fatalf("stderr must be empty: %q", rootOpts.Stderr.(*bytes.Buffer).String())
276+
}
277+
}

0 commit comments

Comments
 (0)