From 5a97020f7e0973b9cf6ff7ecbc950c392405e081 Mon Sep 17 00:00:00 2001 From: heaven Date: Tue, 4 Aug 2026 01:26:04 +0300 Subject: [PATCH] Fix nine silent-error defects: backup and auto-bank stat guards, dotenv warns instead of failing, glossary rows lifetime, langpack and ingest error wrapping, dead applyBanknote removed --- backend/cmd/tmctl/backup.go | 8 +++++- backend/cmd/tmctl/dotenv.go | 20 +++++++++++--- backend/cmd/tmctl/dotenv_test.go | 3 ++- backend/cmd/tmctl/main.go | 4 +-- backend/internal/chunk/ingest.go | 3 ++- backend/internal/lang/langpack.go | 9 +++++-- backend/internal/pipeline/banknote.go | 25 +++++++---------- backend/internal/pipeline/mining.go | 8 +++++- backend/internal/store/glossary.go | 39 ++++++++++++++++----------- backend/internal/store/voice_test.go | 21 ++++++++++----- 10 files changed, 92 insertions(+), 48 deletions(-) diff --git a/backend/cmd/tmctl/backup.go b/backend/cmd/tmctl/backup.go index 4bb575ff..8e78fbc9 100644 --- a/backend/cmd/tmctl/backup.go +++ b/backend/cmd/tmctl/backup.go @@ -8,8 +8,10 @@ package main // backup files without any special-casing here. import ( + "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "time" @@ -54,8 +56,12 @@ func preflightBackup(cfgPath string, w io.Writer) error { if err != nil { return err } + // Only ABSENT means "fresh book". Any other stat error read that way silently skips the guard. if _, err := os.Stat(book.ProjectDB); err != nil { - return nil // first run of a new book: no signed bank exists yet, nothing to back up + if errors.Is(err, fs.ErrNotExist) { + return nil // first run of a new book: no signed bank exists yet, nothing to back up + } + return fmt.Errorf("pre-flight guard: cannot stat the project database %s: %w", book.ProjectDB, err) } path, err := store.BackupSQLite(book.ProjectDB, backupDirFor(book.ProjectDB), backupStamp(time.Now())) if err != nil { diff --git a/backend/cmd/tmctl/dotenv.go b/backend/cmd/tmctl/dotenv.go index fff875f3..6488415e 100644 --- a/backend/cmd/tmctl/dotenv.go +++ b/backend/cmd/tmctl/dotenv.go @@ -2,7 +2,10 @@ package main import ( "bufio" + "errors" + "fmt" "io" + "io/fs" "os" "strings" ) @@ -44,15 +47,26 @@ func parseDotEnv(r io.Reader) []dotenvPair { // already-set variables. Keys come from backend/.env (gitignored); secrets never // end up in configs/the repo. Note: a variable set to an EMPTY string is treated // as unset and will be overridden (current behavior, frozen). -func loadDotEnv(path string) { +// It WARNS but never fails. A missing file is normal — the keys may come from the real environment — +// but a file that exists and cannot be read, or a line os.Setenv refuses (an empty key from a `=VALUE` +// typo), used to be swallowed and surfaced much later as an unexplained 401. Warning rather than +// returning keeps that visible without taking down `status`/`report`/`export`, which are $0 read-only +// projections that must not demand keys at all (D20.4), and lets the remaining valid lines still load. +func loadDotEnv(path string, warn io.Writer) { f, err := os.Open(path) if err != nil { + if !errors.Is(err, fs.ErrNotExist) { + fmt.Fprintf(warn, "tmctl: %s exists but cannot be read (%v) — its keys are NOT loaded\n", path, err) + } return } defer f.Close() for _, p := range parseDotEnv(f) { - if os.Getenv(p.K) == "" { - os.Setenv(p.K, p.V) + if os.Getenv(p.K) != "" { + continue + } + if err := os.Setenv(p.K, p.V); err != nil { + fmt.Fprintf(warn, "tmctl: %s: malformed line ignored, key %q rejected (%v)\n", path, p.K, err) } } } diff --git a/backend/cmd/tmctl/dotenv_test.go b/backend/cmd/tmctl/dotenv_test.go index 2e293a07..f44442d9 100644 --- a/backend/cmd/tmctl/dotenv_test.go +++ b/backend/cmd/tmctl/dotenv_test.go @@ -1,6 +1,7 @@ package main import ( + "io" "os" "path/filepath" "reflect" @@ -41,7 +42,7 @@ func TestLoadDotEnvDoesNotOverride(t *testing.T) { } t.Setenv("TM_TEST_SET", "from_env") t.Setenv("TM_TEST_UNSET", "") // set to EMPTY counts as unset (frozen behaviour) - loadDotEnv(path) + loadDotEnv(path, io.Discard) if got := os.Getenv("TM_TEST_SET"); got != "from_env" { t.Fatalf("an already-set variable must win over .env, got %q", got) } diff --git a/backend/cmd/tmctl/main.go b/backend/cmd/tmctl/main.go index aae375a1..8e9a39d0 100644 --- a/backend/cmd/tmctl/main.go +++ b/backend/cmd/tmctl/main.go @@ -57,8 +57,8 @@ func run() error { return err } - loadDotEnv(filepath.Join(filepath.Dir(inv.cfgPath), ".env")) - loadDotEnv(".env") + loadDotEnv(filepath.Join(filepath.Dir(inv.cfgPath), ".env"), os.Stderr) + loadDotEnv(".env", os.Stderr) ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() diff --git a/backend/internal/chunk/ingest.go b/backend/internal/chunk/ingest.go index 802aa678..d8a4bbd0 100644 --- a/backend/internal/chunk/ingest.go +++ b/backend/internal/chunk/ingest.go @@ -4,6 +4,7 @@ import ( "archive/zip" "bytes" "encoding/xml" + "errors" "fmt" "io" "net/url" @@ -826,7 +827,7 @@ func extractXHTML(data []byte) (string, []RubyReading, error) { for { tt := z.Next() if tt == html.ErrorToken { - if err := z.Err(); err != io.EOF { + if err := z.Err(); !errors.Is(err, io.EOF) { return "", nil, fmt.Errorf("tokenize xhtml: %w", err) } break diff --git a/backend/internal/lang/langpack.go b/backend/internal/lang/langpack.go index a11821cf..e6e57d52 100644 --- a/backend/internal/lang/langpack.go +++ b/backend/internal/lang/langpack.go @@ -334,8 +334,13 @@ func LoadWithOverlay(root, src, tgt, overlayRoot string) (*Pack, error) { // besides the two expected subdirectories, is an operator mistake — and the fold below would swallow it // in silence. langpack_extend is an EXPLICIT opt-in in book.yaml: if it is set, the caller asserts that a // private canon exists, so a missing/typo'd path must stop the load, never degrade recall quietly. - if fi, serr := os.Stat(overlayRoot); serr != nil || !fi.IsDir() { - return nil, fmt.Errorf("langpack %q overlay root %s is not a readable directory (langpack_extend is an explicit opt-in — a missing or mistyped path would silently drop the book's private canon): %v", pair, overlayRoot, serr) + const overlayWhy = "langpack_extend is an explicit opt-in — a missing or mistyped path would silently drop the book's private canon" + fi, serr := os.Stat(overlayRoot) + if serr != nil { + return nil, fmt.Errorf("langpack %q overlay root %s is not readable (%s): %w", pair, overlayRoot, overlayWhy, serr) + } + if !fi.IsDir() { + return nil, fmt.Errorf("langpack %q overlay root %s is not a directory (%s)", pair, overlayRoot, overlayWhy) } rootEnts, rerr := os.ReadDir(overlayRoot) if rerr != nil { diff --git a/backend/internal/pipeline/banknote.go b/backend/internal/pipeline/banknote.go index 32abda71..36f59fb3 100644 --- a/backend/internal/pipeline/banknote.go +++ b/backend/internal/pipeline/banknote.go @@ -291,21 +291,16 @@ var bankTokenBudget = bankMaxLines*bankWorstLineTokens + EstimateTokens(bankSepa // The literal below IS the arithmetic — it is measured, not asserted (see TestBankTokenBudgetIsDerived). var bankWorstLineTokens = EstimateTokens(strings.Repeat("蛊", 8) + "\t" + strings.Repeat("я", 40) + "\tnickname\n") -// applyBanknote slices the banknote block off a translator draft (integration points 1–5, 8). It runs -// ONLY on the translator role with the channel enabled — the editor never emits banknotes, and a normal -// draft has no separator (the slice is then a no-op that returns the raw text unchanged, so a -// banknote-OFF run is byte-identical). When a ⟦TM-BANK-v1⟧ block IS present it returns the CLEANED -// translation (what classify/coverage/echo/editor/export all see — points 2/3/4) plus that same cleaned -// text as the `stripped` export to commit as a derived checkpoint (point 8), and the per-chunk telemetry -// (point 10). Candidates are PARSED/accepted only under finish=="stop" (the finish=stop-only gate §4б — -// a truncated block is not trusted); the slice itself runs regardless, so the length/echo classify never -// sees the footnote's Han src column. Deterministic (a pure function of the raw text + gate state). -func (r *Runner) applyBanknote(role, rawText, finish, chunkSource string) (clean, stripped string, flags bankFlags) { - clean, stripped, flags, _ = r.applyBanknoteWithEntries(role, rawText, finish, chunkSource) - return clean, stripped, flags -} - -// applyBanknoteWithEntries is applyBanknote plus the PARSED entries — the WHAT itself. +// applyBanknoteWithEntries slices the banknote block off a translator draft (integration points 1–5, 8) +// and returns the PARSED entries — the WHAT itself. It runs ONLY on the translator role with the channel +// enabled — the editor never emits banknotes, and a normal draft has no separator (the slice is then a +// no-op that returns the raw text unchanged, so a banknote-OFF run is byte-identical). When a ⟦TM-BANK-v1⟧ +// block IS present it returns the CLEANED translation (what classify/coverage/echo/editor/export all see — +// points 2/3/4) plus that same cleaned text as the `stripped` export to commit as a derived checkpoint +// (point 8), and the per-chunk telemetry (point 10). Candidates are PARSED/accepted only under +// finish=="stop" (the finish=stop-only gate §4б — a truncated block is not trusted); the slice itself runs +// regardless, so the length/echo classify never sees the footnote's Han src column. Deterministic (a pure +// function of the raw text + gate state). // // Until the mini-run of 25.07 the entries were dropped into `_` here (D39.36): the model proposed a // rendering for every new term, the parser accepted it, and the code kept only the counters. The owner diff --git a/backend/internal/pipeline/mining.go b/backend/internal/pipeline/mining.go index 60e0bf90..1a8819e5 100644 --- a/backend/internal/pipeline/mining.go +++ b/backend/internal/pipeline/mining.go @@ -2,7 +2,9 @@ package pipeline import ( "context" + "errors" "fmt" + "io/fs" "os" "sort" "strings" @@ -381,8 +383,12 @@ func (r *Runner) autoBankPath() string { return r.Book.ProjectDB + ".auto-bank.y // whatever the emission wrote (auto/draft). A file hand-edited to say `approved` is refused loudly, // because that would be a signature nobody gave. func (r *Runner) loadAutoBank(signed []store.GlossaryEntry) (rows []store.GlossaryEntry, dropped []string, err error) { + // Only ABSENT means "auto mode has not run"; an unreadable file must not drop the mined rows. if _, serr := os.Stat(r.autoBankPath()); serr != nil { - return nil, nil, nil // absent → the auto mode has not run yet (or the book never uses it) + if errors.Is(serr, fs.ErrNotExist) { + return nil, nil, nil // the auto mode has not run yet (or the book never uses it) + } + return nil, nil, fmt.Errorf("pipeline: stat auto-bank %s: %w", r.autoBankPath(), serr) } entries, err := membank.LoadGlossarySeed(r.autoBankPath()) if err != nil { diff --git a/backend/internal/store/glossary.go b/backend/internal/store/glossary.go index 80d93d76..756cf212 100644 --- a/backend/internal/store/glossary.go +++ b/backend/internal/store/glossary.go @@ -1,6 +1,7 @@ package store import ( + "context" "database/sql" "errors" ) @@ -135,25 +136,10 @@ func (s *Store) ReplaceBank(bookID string, entries []GlossaryEntry, voices []Voi defer tx.Rollback() // Snapshot the prior (src,sense,window)→dst to detect editorial-time changes (B1). - prior := map[[4]any]string{} - rows, err := tx.QueryContext(ctx, `SELECT src, sense, since_ch, until_ch, dst FROM glossary WHERE book_id = ?`, bookID) + prior, err := priorRenderings(ctx, tx, bookID) if err != nil { return err } - for rows.Next() { - var src, sense, dst string - var since, until int - if err := rows.Scan(&src, &sense, &since, &until, &dst); err != nil { - rows.Close() - return err - } - prior[[4]any{src, sense, since, until}] = dst - } - if err := rows.Err(); err != nil { - rows.Close() - return err - } - rows.Close() for _, table := range []string{"glossary_aliases", "glossary", "voice_profiles", "address_pairs"} { if _, err := tx.ExecContext(ctx, `DELETE FROM `+table+` WHERE book_id = ?`, bookID); err != nil { @@ -404,3 +390,24 @@ func boolToInt(b bool) int { } return 0 } + +// priorRenderings reads the (src,sense,window)→dst map a replace overwrites. Separate function so the +// rows close before the caller's DELETEs run on the same tx — a tx holds one connection. +func priorRenderings(ctx context.Context, tx *sql.Tx, bookID string) (map[[4]any]string, error) { + rows, err := tx.QueryContext(ctx, `SELECT src, sense, since_ch, until_ch, dst FROM glossary WHERE book_id = ?`, bookID) + if err != nil { + return nil, err + } + defer rows.Close() + + prior := map[[4]any]string{} + for rows.Next() { + var src, sense, dst string + var since, until int + if err := rows.Scan(&src, &sense, &since, &until, &dst); err != nil { + return nil, err + } + prior[[4]any{src, sense, since, until}] = dst + } + return prior, rows.Err() +} diff --git a/backend/internal/store/voice_test.go b/backend/internal/store/voice_test.go index 3e955443..8da3c08b 100644 --- a/backend/internal/store/voice_test.go +++ b/backend/internal/store/voice_test.go @@ -37,13 +37,22 @@ func TestMigrationUpgradesAnOlderDatabase(t *testing.T) { t.Fatal(err) } // Premise check: at the OLD head the pack-19 schema must genuinely be absent, or this test would - // prove nothing about upgrading. - if _, qerr := raw.Query(`SELECT n_voice_flags FROM retrieval_state`); qerr == nil { - t.Fatal("the old schema already has the pack-19 columns — the upgrade is not being exercised") - } - if _, qerr := raw.Query(`SELECT 1 FROM voice_profiles`); qerr == nil { - t.Fatal("the old schema already has voice_profiles — the upgrade is not being exercised") + // prove nothing about upgrading. A successful query means it is already there. + mustNotQuery := func(query, why string) { + t.Helper() + rows, qerr := raw.Query(query) + if qerr != nil { + return // the column/table is absent — exactly the premise this test needs + } + defer rows.Close() + if rows.Err() == nil { + t.Fatal(why) + } } + mustNotQuery(`SELECT n_voice_flags FROM retrieval_state`, + "the old schema already has the pack-19 columns — the upgrade is not being exercised") + mustNotQuery(`SELECT 1 FROM voice_profiles`, + "the old schema already has voice_profiles — the upgrade is not being exercised") if err := raw.Close(); err != nil { t.Fatal(err) }