Merge pull request #37 from jjuanrivvera/fix/emit-cache-and-edge-cases

fix: harden the emit idempotency cache and surface silent truncations

Author: jjuanrivvera99

CONTRIBUTING.md

@@ -58,7 +58,11 @@ A resource needs three files and no edits to shared code:
    ```
 
 3. `internal/api/<resource>_test.go` — an httptest-based service test (reuse the
-   `newTestClient` helper).
+   `newTestClient` helper). Test what is **unique** to the resource — special
+   field types, custom actions, odd response shapes — not the generic CRUD
+   plumbing, which is already covered once by the `Resource[T]` tests
+   (`resource_test.go`, `client_failures_test.go`). A List/Get happy-path pair
+   adds volume, not signal.
 
 Custom actions (e.g. `void`, `email`) go through the `Extra` hook using
 `NewActionCmd` / `NewCollectionActionCmd`. Non-CRUD resources (singletons,
@@ -82,6 +86,16 @@ reports) build a plain cobra command using `client.GetInto/PostInto/PutInto`.
   `internal/api`).
 - Prefer `require` for fatal assertions, `assert` for the rest.
 - Keep coverage healthy — the suite sits above 80%; new code should ship tests.
+- **Test failure paths, not just happy paths.** Every parse of external state
+  (API bodies, config files, caches) needs a test with corrupt input; every
+  batch operation needs a partial-failure test asserting counts and a non-zero
+  exit. Coverage measures execution, not assertion quality — a swallowed error
+  can be 100% "covered" and still hide a bug.
+- The flexible JSON types have fuzzers with value-level properties
+  (`internal/api/fuzz_test.go`); run them after touching a decoder:
+  `go test ./internal/api -fuzz '^FuzzID$' -fuzztime 30s` (likewise `FuzzInt`,
+  `FuzzMoney`, `FuzzStringOrSlice`). Counterexamples land in `testdata/fuzz/`
+  and become permanent regression cases — commit them.
 
 ## Reporting bugs & security issues
 

commands/auth.go

@@ -109,7 +109,10 @@ func newAuthStatusCmd() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			cfg, _ := config.Load()
+			cfg, err := config.Load()
+			if err != nil {
+				return err
+			}
 			profile := cfg.ActiveProfileName(flagProfile)
 			r := cfg.Resolve(profile)
 			out := cmd.OutOrStdout()

commands/country.go

@@ -36,5 +36,7 @@ func cacheCountry(cfg *config.Config, profile, country string) {
 	}
 	p.Country = country
 	cfg.SetProfile(p)
+	// Best-effort by design: the cache only saves a future detection call, so
+	// a failed write must never break the command that triggered it.
 	_ = cfg.Save()
 }

commands/emit.go

@@ -2,6 +2,7 @@ package commands
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -60,9 +61,14 @@ emission is NOT idempotent on Alegra's side, so this prevents duplicates.`,
 				return fmt.Errorf("no invoices to emit (pass ids or --all)")
 			}
 
-			// 2. Idempotency guard.
+			// 2. Idempotency guard. A cache that exists but cannot be read is a
+			// hard stop: proceeding with an empty guard could re-emit invoices
+			// that were already stamped (a fiscal duplicate, not undoable).
 			profile := currentProfileName()
-			cache, _ := loadEmitCache()
+			cache, cerr := loadEmitCache()
+			if cerr != nil {
+				return fmt.Errorf("cannot read the emission idempotency cache: %w\nInspect or remove %s, then retry", cerr, emitCachePath())
+			}
 			todo, skipped := filterEmitted(ids, cache, profile, force)
 			out := cmd.OutOrStdout()
 			if len(skipped) > 0 {
@@ -92,9 +98,14 @@ emission is NOT idempotent on Alegra's side, so this prevents duplicates.`,
 					emitted++
 				}
 				fmt.Fprintf(out, "stamped: %v\n", batch)
+				// Persist after every batch, and stop on failure: stamping more
+				// invoices the guard cannot record would risk re-emission on the
+				// next run.
+				if werr := saveEmitCache(cache); werr != nil {
+					return fmt.Errorf("stamped %v but could not record them in the idempotency cache: %w\nRecord these ids in %s before re-running, or a re-run may emit them twice", batch, werr, emitCachePath())
+				}
 			}
 			if !flagDryRun {
-				_ = saveEmitCache(cache)
 				fmt.Fprintf(out, "Emitted %d invoice(s); %d batch(es) failed.\n", emitted, failedChunks)
 			}
 			if failedChunks > 0 {
@@ -146,26 +157,54 @@ func emitCachePath() string {
 	return filepath.Join(filepath.Dir(config.DefaultPath()), "emitted.json")
 }
 
+// loadEmitCache reads the emitted-ids cache. A missing file is a fresh start;
+// an unreadable or corrupt file is an error — silently treating it as empty
+// would drop the idempotency guard and allow double emission.
 func loadEmitCache() (map[string]bool, error) {
 	cache := map[string]bool{}
 	data, err := os.ReadFile(emitCachePath()) //nolint:gosec // path under config dir
+	if errors.Is(err, os.ErrNotExist) {
+		return cache, nil
+	}
 	if err != nil {
-		return cache, err
+		return nil, err
+	}
+	if err := json.Unmarshal(data, &cache); err != nil {
+		return nil, fmt.Errorf("corrupt cache %s: %w", emitCachePath(), err)
 	}
-	_ = json.Unmarshal(data, &cache)
 	return cache, nil
 }
 
+// saveEmitCache persists atomically (write temp + rename): a crash mid-write
+// must never leave a torn emitted.json, which would read as corrupt and block
+// (or, worse, lose) the guard.
 func saveEmitCache(cache map[string]bool) error {
 	path := emitCachePath()
-	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0o700); err != nil {
 		return err
 	}
 	data, err := json.Marshal(cache)
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(path, data, 0o600)
+	tmp, err := os.CreateTemp(dir, "emitted-*.json")
+	if err != nil {
+		return err
+	}
+	defer func() { _ = os.Remove(tmp.Name()) }() // no-op once renamed
+	if err := tmp.Chmod(0o600); err != nil {
+		_ = tmp.Close()
+		return err
+	}
+	if _, err := tmp.Write(data); err != nil {
+		_ = tmp.Close()
+		return err
+	}
+	if err := tmp.Close(); err != nil {
+		return err
+	}
+	return os.Rename(tmp.Name(), path)
 }
 
 // currentProfileName returns the active profile name for cache scoping.

commands/failures_test.go

@@ -0,0 +1,165 @@
+package commands
+
+import (
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"sync/atomic"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/zalando/go-keyring"
+
+	"github.com/jjuanrivvera/alegra-cli/internal/config"
+)
+
+// These tests exercise the failure paths the integration suite never hit: the
+// fake servers previously only returned 200/404, so partial batch failures,
+// API errors mid-operation, and persistence failures were all untested.
+
+// failureTestEnv points the CLI at srv with env-only credentials and an
+// isolated config dir, returning that dir.
+func failureTestEnv(t *testing.T, srv *httptest.Server) string {
+	t.Helper()
+	dir := t.TempDir()
+	t.Setenv(config.EnvBaseURL, srv.URL)
+	t.Setenv(config.EnvEmail, "e@x.com")
+	t.Setenv(config.EnvToken, "tok")
+	t.Setenv(config.EnvProfile, "")
+	t.Setenv(config.EnvConfig, filepath.Join(dir, "config.yaml"))
+	return dir
+}
+
+func TestImport_PartialFailureFailsLoudly(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		body, _ := io.ReadAll(r.Body)
+		var m map[string]any
+		_ = json.Unmarshal(body, &m)
+		if m["name"] == "Bad" {
+			w.WriteHeader(http.StatusBadRequest)
+			_, _ = w.Write([]byte(`{"message":"name is invalid","code":400}`))
+			return
+		}
+		_, _ = w.Write([]byte(`{"id":"9","name":"ok"}`))
+	}))
+	t.Cleanup(srv.Close)
+	dir := failureTestEnv(t, srv)
+
+	csvFile := filepath.Join(dir, "rows.csv")
+	require.NoError(t, os.WriteFile(csvFile, []byte("name\nGood\nBad\nAlsoGood\n"), 0o600))
+
+	out, err := runRoot(t, "contacts", "import", "-f", csvFile)
+	// A partial import must fail the command (exit code), report which row
+	// broke, and still account for the rows that were created.
+	require.Error(t, err, "partial failure must not look like success to pipelines")
+	assert.Contains(t, err.Error(), "1 row(s) failed")
+	assert.Contains(t, out, "[row 2] FAILED")
+	assert.Contains(t, out, "Imported 2, failed 1")
+}
+
+func TestEmit_BatchAPIErrorFailsAndKeepsCacheClean(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, "/invoices/stamp") {
+			w.WriteHeader(http.StatusBadRequest) // 4xx: not retried, fails fast
+			_, _ = w.Write([]byte(`{"message":"EPR001: certificado vencido"}`))
+			return
+		}
+		_, _ = w.Write([]byte(`{}`))
+	}))
+	t.Cleanup(srv.Close)
+	dir := failureTestEnv(t, srv)
+
+	out, err := runRoot(t, "invoices", "emit", "7", "--force")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "1 batch(es) failed")
+	assert.Contains(t, out, "FAILED")
+
+	// A failed batch must never be recorded as emitted.
+	data, rerr := os.ReadFile(filepath.Join(dir, "emitted.json"))
+	if rerr == nil {
+		assert.NotContains(t, string(data), ":7", "failed invoice must not be marked emitted")
+	}
+}
+
+func TestEmit_CacheSaveFailureStopsEmission(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("read-only directory permissions are not enforced on Windows")
+	}
+	var stamps int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, "/invoices/stamp") {
+			atomic.AddInt32(&stamps, 1)
+			_, _ = w.Write([]byte(`{"stamped":true}`))
+			return
+		}
+		_, _ = w.Write([]byte(`{}`))
+	}))
+	t.Cleanup(srv.Close)
+	dir := failureTestEnv(t, srv)
+
+	// Make the config dir unwritable so the post-batch cache save fails.
+	require.NoError(t, os.Chmod(dir, 0o500))
+	t.Cleanup(func() { _ = os.Chmod(dir, 0o700) })
+
+	// 12 ids → 2 batches. The save failure after batch 1 must stop the run
+	// before batch 2 is stamped, and the error must name the affected ids.
+	ids := []string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12"}
+	out, err := runRoot(t, append([]string{"invoices", "emit", "--force"}, ids...)...)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "could not record them in the idempotency cache")
+	assert.Contains(t, err.Error(), "1") // affected ids are listed
+	assert.Contains(t, out, "stamped")
+	assert.Equal(t, int32(1), atomic.LoadInt32(&stamps), "must stop stamping once the guard cannot record progress")
+}
+
+func TestDelete_AbortsWithoutConfirmation(t *testing.T) {
+	var deletes int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if r.Method == http.MethodDelete {
+			atomic.AddInt32(&deletes, 1)
+		}
+		_, _ = w.Write([]byte(`{}`))
+	}))
+	t.Cleanup(srv.Close)
+	failureTestEnv(t, srv)
+
+	// Piped/empty stdin without --yes: the prompt cannot be answered, so the
+	// delete must abort — and the DELETE request must never be sent.
+	rootCmd.SetIn(strings.NewReader(""))
+	t.Cleanup(func() { rootCmd.SetIn(nil) })
+
+	_, err := runRoot(t, "contacts", "delete", "1")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "aborted")
+	assert.Zero(t, atomic.LoadInt32(&deletes), "no DELETE may be sent without confirmation")
+}
+
+func TestAuthLogin_NeverPersistsTokenToConfig(t *testing.T) {
+	keyring.MockInit()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"name":"Tester","applicationVersion":"colombia"}`))
+	}))
+	t.Cleanup(srv.Close)
+	dir := failureTestEnv(t, srv)
+	t.Setenv(config.EnvToken, "") // force login to use the flag token
+
+	const secret = "super-secret-token"
+	_, err := runRoot(t, "auth", "login", "--email", "a@x.com", "--token", secret, "--profile", "p1")
+	require.NoError(t, err)
+
+	// The token belongs in the keyring only; the YAML must never contain it.
+	data, rerr := os.ReadFile(filepath.Join(dir, "config.yaml"))
+	require.NoError(t, rerr)
+	assert.NotContains(t, string(data), secret, "plaintext token leaked into config.yaml")
+}

commands/generic.go

@@ -131,6 +131,12 @@ func buildResourceCmd[T any](sp resourceSpec[T]) *cobra.Command {
 
 // --- list ---
 
+// droppedListFilters records resource filters that were skipped at registration
+// because of an empty definition or a flag collision. The CLI must never panic
+// at init over a bad resource definition, so the mistake is surfaced by a
+// registry test (TestNoListFiltersAreSilentlyDropped) failing CI instead.
+var droppedListFilters []string
+
 // reservedListFlags are the built-in flag names on every `list` subcommand;
 // resource-specific filters that collide with these are skipped.
 var reservedListFlags = map[string]bool{
@@ -245,9 +251,15 @@ func newListCmd[T any](sp resourceSpec[T]) *cobra.Command {
 	fs.StringVar(&lf.orderDir, "order-direction", "", "Sort direction: ASC or DESC")
 	// Register resource-specific filters, skipping any that would collide with a
 	// built-in list flag or a previously declared filter (defensive: a bad
-	// resource definition must never panic the whole CLI at init).
+	// resource definition must never panic the whole CLI at init). Skips are
+	// recorded so the registry test fails CI instead of losing filters silently.
 	for _, f := range sp.ListFilters {
-		if f.Flag == "" || f.Query == "" || reservedListFlags[f.Flag] || fs.Lookup(f.Flag) != nil {
+		if f.Flag == "" || f.Query == "" {
+			droppedListFilters = append(droppedListFilters, fmt.Sprintf("%s: filter %+v has an empty flag or query", sp.Use, f))
+			continue
+		}
+		if reservedListFlags[f.Flag] || fs.Lookup(f.Flag) != nil {
+			droppedListFilters = append(droppedListFilters, fmt.Sprintf("%s: --%s collides with a built-in or duplicate flag", sp.Use, f.Flag))
 			continue
 		}
 		filterVals[f.Query] = fs.String(f.Flag, "", f.Usage)
@@ -415,18 +427,21 @@ failures are reported and do not stop the run.`,
 					setDotPath(body, field, inferValue(cell))
 				}
 				if flagDryRun {
-					raw, _ := json.Marshal(body)
+					raw, merr := json.Marshal(body)
+					if merr != nil {
+						failed++
+						fmt.Fprintf(cmd.ErrOrStderr(), "[row %d] FAILED: cannot encode body: %v\n", i+1, merr)
+						continue
+					}
 					fmt.Fprintf(out, "[row %d] would create: %s\n", i+1, raw)
 					continue
 				}
-				item, cerr := res.Create(cmd.Context(), body)
-				if cerr != nil {
+				if _, cerr := res.Create(cmd.Context(), body); cerr != nil {
 					failed++
 					fmt.Fprintf(cmd.ErrOrStderr(), "[row %d] FAILED: %v\n", i+1, cerr)
 					continue
 				}
 				created++
-				_ = item
 				fmt.Fprintf(out, "[row %d] created\n", i+1)
 			}
 			if !flagDryRun {
@@ -541,7 +556,10 @@ func newCreateCmd[T any](sp resourceSpec[T]) *cobra.Command {
 			if draft {
 				if m, ok := bodyToMap(body); ok {
 					delete(m, "stamp")
-					body, _ = json.Marshal(m)
+					body, err = json.Marshal(m)
+					if err != nil {
+						return fmt.Errorf("re-encoding body after stripping stamp: %w", err)
+					}
 				}
 			}
 			if !noValidate {

commands/generic_more_test.go

@@ -87,3 +87,11 @@ func TestConfirm(t *testing.T) {
 		assert.False(t, confirm(cmd, "proceed"), "input %q", in)
 	}
 }
+
+// Every registered resource builds its list command at init, so by the time
+// tests run any filter dropped over an empty definition or a flag collision is
+// already recorded. An entry here means a resource silently lost a filter —
+// rename the flag in the resource definition.
+func TestNoListFiltersAreSilentlyDropped(t *testing.T) {
+	assert.Empty(t, droppedListFilters)
+}