Plan: Fix config library issues (priority order) #3

New issue

Open

opened 2026-09-20 00:26:19 +00:00 by agent · 3 comments

agent commented 2026-09-20 00:26:19 +00:00

Member

Copy link

Plan: Fix config library issues (priority order)

Fix order

1. InitWithTemplate error swallowing (Forgejo #1) — tier-1-easy
2. Add float support to setFromString + coerceDefault — tier-1-easy
3. Fix stale docs (optional → omitempty, integration status) — tier-0-trivial
4. structToMap omitempty propagation — tier-2-medium (design decision needed)
5. RenderTemplate missing key handling — tier-1-easy (design decision needed)
6. MergeWithRules wildcard matching for nested fields — tier-3-hard (design decision needed)
7. write.go dir permissions umask — tier-0-trivial

Fix details

1. InitWithTemplate error swallowing (core/load.go:61-82)

Problem: Returns (cfg, nil) even when template rendering, dir creation, or file writing fails.

Fix: Return errors from the template-writing block. The cascade load before it still runs (errors from System/User are intentionally ignored for missing files), but template writing failures are returned.

func InitWithTemplate[T any](a *App, defaults T, tmpl string) (T, error) {
    cfg := Defaults[T]()
    sysCfg, _ := System[T](a)
    usrCfg, _ := User[T](a)
    cfg = UseConfig(cfg, sysCfg)
    cfg = UseConfig(cfg, usrCfg)

    if tmpl != "" {
        if _, err := os.Stat(a.UserPath()); os.IsNotExist(err) {
            rendered, rerr := RenderTemplate(tmpl, defaults)
            if rerr != nil {
                return cfg, fmt.Errorf("config: render template: %w", rerr)
            }
            if err := os.MkdirAll(filepath.Dir(a.UserPath()), 0o700); err != nil {
                return cfg, fmt.Errorf("config: mkdir template dir: %w", err)
            }
            if err := os.WriteFile(a.UserPath(), []byte(rendered), 0o644); err != nil {
                return cfg, fmt.Errorf("config: write template: %w", err)
            }
            // Reload after writing template.
            usrCfg, _ = User[T](a)
            cfg = UseConfig(cfg, usrCfg)
        }
    }
    return cfg, nil
}

Files: core/load.go
Tests: Add test that triggers template write error (read-only dir or bad template), verify error is returned.
Closes: Forgejo #1

2. Add float support to setFromString + coerceDefault

Problem: setFromString (load.go) and coerceDefault (write.go) don't handle reflect.Float32/Float64. Fields with default:"3.14" silently get no default.

Fix in core/load.go setFromString:

case reflect.Float32, reflect.Float64:
    n, err := strconv.ParseFloat(val, 64)
    if err != nil {
        return err
    }
    field.SetFloat(n)

Fix in core/write.go coerceDefault:

case reflect.Float32, reflect.Float64:
    n, err := strconv.ParseFloat(f.Default, 64)
    if err != nil {
        return nil
    }
    return n

Fix in core/env.go coerceAndSet:

case reflect.Float32, reflect.Float64:
    n, err := strconv.ParseFloat(val, 64)
    if err != nil {
        return err
    }
    field.SetFloat(n)

Files: core/load.go, core/write.go, core/env.go
Tests: Add float fields to test structs; test default:"3.14", env var PREFIX__FLOAT_FIELD=2.718, and write round-trip.

3. Fix stale docs

Files:

• README.md line 27: optional → omitempty
• docs/residual-overview.md line 16: optional → omitempty
• docs/residual-overview.md lines 7, 23: Update integration status ("integrated into core/ via config/ import path")

4. structToMap omitempty propagation (design decision)

Problem: structToMap always writes all nested fields including zero-value ones with omitempty. The omitempty hint only works at the top level via buildMarshalOutput.

Question for user: Should structToMap also skip zero-value fields that have omitempty? This would mean nested struct sections could have fewer keys in TOML. Or is "always write all nested fields" the intended behavior?

If yes: Add omitempty check to structToMap — skip fields where f.OmitEmpty && field.IsZero() && f.Default == "".

If no: Document that omitempty only applies at the top level.

5. RenderTemplate missing key handling (design decision)

Problem: Missing placeholder keys render as literal <not found> in the config file.

Options:

• A) Return an error if any placeholder is unresolved → strict, prevents bad config files
• B) Keep <not found> but log a warning → permissive, useful for templates with optional sections
• C) Leave as-is, document the behavior

Recommendation: Option A — return an error. A template that references a non-existent key is a bug. The caller can catch and handle it.

6. MergeWithRules wildcard matching for nested fields (design decision)

Problem: resolveFieldMode receives f.Name which is the leaf conf tag name (e.g. "host"), not a dotted path. Patterns like "database.*" can never match because there's no path context.

Options:

• A) Skip for now — MergeWithRules isn't used in production yet, and the wildcard feature is untested in real use. Document the limitation.
• B) Fix — pass the full dotted path to resolveFieldMode. Requires inspectFieldsFromType to include parent path context, or the caller to build it.

Recommendation: Option A for now. The feature is experimental, not used in production, and fixing it properly requires restructuring how field metadata is passed through the merge pipeline.

7. write.go dir permissions umask

Problem: os.MkdirAll(filepath.Dir(path), 0o700) ignores umask.

Fix: Use 0o777 and let umask do its job, or use os.MkdirAll with a mode that respects the process umask. The current 0o700 is overly restrictive on shared systems.

Files: core/write.go line 50

Verification

After all fixes:

cd config && GOWORK=off go build ./... && GOWORK=off go test -count=1 ./... && GOWORK=off go vet ./...

Commit strategy

One commit per fix (logical atomicity), or group:

• Fix 1 + 2 together (both are core/ code fixes)
• Fix 3 alone (docs only)
• Fixes 4-7 as individual commits based on design decisions

## Plan: Fix config library issues (priority order) ### Fix order 1. **InitWithTemplate error swallowing** (Forgejo #1) — tier-1-easy 2. **Add float support to setFromString + coerceDefault** — tier-1-easy 3. **Fix stale docs** (`optional` → `omitempty`, integration status) — tier-0-trivial 4. **structToMap omitempty propagation** — tier-2-medium (design decision needed) 5. **RenderTemplate missing key handling** — tier-1-easy (design decision needed) 6. **MergeWithRules wildcard matching for nested fields** — tier-3-hard (design decision needed) 7. **write.go dir permissions umask** — tier-0-trivial ### Fix details #### 1. InitWithTemplate error swallowing (`core/load.go:61-82`) **Problem:** Returns `(cfg, nil)` even when template rendering, dir creation, or file writing fails. **Fix:** Return errors from the template-writing block. The cascade load before it still runs (errors from `System`/`User` are intentionally ignored for missing files), but template writing failures are returned. ```go func InitWithTemplate[T any](a *App, defaults T, tmpl string) (T, error) { cfg := Defaults[T]() sysCfg, _ := System[T](a) usrCfg, _ := User[T](a) cfg = UseConfig(cfg, sysCfg) cfg = UseConfig(cfg, usrCfg) if tmpl != "" { if _, err := os.Stat(a.UserPath()); os.IsNotExist(err) { rendered, rerr := RenderTemplate(tmpl, defaults) if rerr != nil { return cfg, fmt.Errorf("config: render template: %w", rerr) } if err := os.MkdirAll(filepath.Dir(a.UserPath()), 0o700); err != nil { return cfg, fmt.Errorf("config: mkdir template dir: %w", err) } if err := os.WriteFile(a.UserPath(), []byte(rendered), 0o644); err != nil { return cfg, fmt.Errorf("config: write template: %w", err) } // Reload after writing template. usrCfg, _ = User[T](a) cfg = UseConfig(cfg, usrCfg) } } return cfg, nil } ``` **Files:** `core/load.go` **Tests:** Add test that triggers template write error (read-only dir or bad template), verify error is returned. **Closes:** Forgejo #1 #### 2. Add float support to `setFromString` + `coerceDefault` **Problem:** `setFromString` (load.go) and `coerceDefault` (write.go) don't handle `reflect.Float32`/`Float64`. Fields with `default:"3.14"` silently get no default. **Fix in `core/load.go` `setFromString`:** ```go case reflect.Float32, reflect.Float64: n, err := strconv.ParseFloat(val, 64) if err != nil { return err } field.SetFloat(n) ``` **Fix in `core/write.go` `coerceDefault`:** ```go case reflect.Float32, reflect.Float64: n, err := strconv.ParseFloat(f.Default, 64) if err != nil { return nil } return n ``` **Fix in `core/env.go` `coerceAndSet`:** ```go case reflect.Float32, reflect.Float64: n, err := strconv.ParseFloat(val, 64) if err != nil { return err } field.SetFloat(n) ``` **Files:** `core/load.go`, `core/write.go`, `core/env.go` **Tests:** Add float fields to test structs; test `default:"3.14"`, env var `PREFIX__FLOAT_FIELD=2.718`, and write round-trip. #### 3. Fix stale docs **Files:** - `README.md` line 27: `optional` → `omitempty` - `docs/residual-overview.md` line 16: `optional` → `omitempty` - `docs/residual-overview.md` lines 7, 23: Update integration status ("integrated into core/ via `config/` import path") #### 4. structToMap omitempty propagation (design decision) **Problem:** `structToMap` always writes all nested fields including zero-value ones with `omitempty`. The `omitempty` hint only works at the top level via `buildMarshalOutput`. **Question for user:** Should `structToMap` also skip zero-value fields that have `omitempty`? This would mean nested struct sections could have fewer keys in TOML. Or is "always write all nested fields" the intended behavior? **If yes:** Add `omitempty` check to `structToMap` — skip fields where `f.OmitEmpty && field.IsZero() && f.Default == ""`. **If no:** Document that `omitempty` only applies at the top level. #### 5. RenderTemplate missing key handling (design decision) **Problem:** Missing placeholder keys render as literal `<not found>` in the config file. **Options:** - **A)** Return an error if any placeholder is unresolved → strict, prevents bad config files - **B)** Keep `<not found>` but log a warning → permissive, useful for templates with optional sections - **C)** Leave as-is, document the behavior **Recommendation:** Option A — return an error. A template that references a non-existent key is a bug. The caller can catch and handle it. #### 6. MergeWithRules wildcard matching for nested fields (design decision) **Problem:** `resolveFieldMode` receives `f.Name` which is the leaf conf tag name (e.g. `"host"`), not a dotted path. Patterns like `"database.*"` can never match because there's no path context. **Options:** - **A)** Skip for now — `MergeWithRules` isn't used in production yet, and the wildcard feature is untested in real use. Document the limitation. - **B)** Fix — pass the full dotted path to `resolveFieldMode`. Requires `inspectFieldsFromType` to include parent path context, or the caller to build it. **Recommendation:** Option A for now. The feature is experimental, not used in production, and fixing it properly requires restructuring how field metadata is passed through the merge pipeline. #### 7. write.go dir permissions umask **Problem:** `os.MkdirAll(filepath.Dir(path), 0o700)` ignores umask. **Fix:** Use `0o777` and let umask do its job, or use `os.MkdirAll` with a mode that respects the process umask. The current `0o700` is overly restrictive on shared systems. **Files:** `core/write.go` line 50 ### Verification After all fixes: ```bash cd config && GOWORK=off go build ./... && GOWORK=off go test -count=1 ./... && GOWORK=off go vet ./... ``` ### Commit strategy One commit per fix (logical atomicity), or group: - Fix 1 + 2 together (both are `core/` code fixes) - Fix 3 alone (docs only) - Fixes 4-7 as individual commits based on design decisions

agent added the

proj-config-lib

tier-1-easy

labels 2026-09-20 00:26:26 +00:00

agent commented 2026-09-20 00:37:49 +00:00

Author

Member

Copy link

Design decisions (confirmed by user)

Fix 4 — structToMap omitempty propagation

Decision: Fix it. omitempty should function like it does in other parsers — when writing, zero-value fields with omitempty are omitted from the output. This applies at all nesting levels, not just top-level. When reading, parsers should not error on missing values (already handled by the shadow type decoder).

Fix 5 — RenderTemplate missing keys

Decision: Option A — return error. If a template references a key that doesn't exist in the config struct, that's a bug. Return an error. The caller can handle it.

Fix 6 — MergeWithRules wildcard matching

Decision: Skip for now. Noted as a backburner item. MergeWithRules isn't used in production yet, and the wildcard feature is untested in real use. Document the limitation.

Implementation order

1. InitWithTemplate error swallowing (return errors)
2. Float support (setFromString, coerceDefault, coerceAndSet)
3. Stale docs (optional → omitempty, integration status)
4. structToMap omitempty propagation
5. RenderTemplate missing key error
6. Dir permissions umask
7. MergeWithRules wildcards — document limitation only

## Design decisions (confirmed by user) ### Fix 4 — structToMap omitempty propagation **Decision: Fix it.** omitempty should function like it does in other parsers — when writing, zero-value fields with omitempty are omitted from the output. This applies at all nesting levels, not just top-level. When reading, parsers should not error on missing values (already handled by the shadow type decoder). ### Fix 5 — RenderTemplate missing keys **Decision: Option A — return error.** If a template references a key that doesn't exist in the config struct, that's a bug. Return an error. The caller can handle it. ### Fix 6 — MergeWithRules wildcard matching **Decision: Skip for now.** Noted as a backburner item. `MergeWithRules` isn't used in production yet, and the wildcard feature is untested in real use. Document the limitation. ### Implementation order 1. InitWithTemplate error swallowing (return errors) 2. Float support (setFromString, coerceDefault, coerceAndSet) 3. Stale docs (optional → omitempty, integration status) 4. structToMap omitempty propagation 5. RenderTemplate missing key error 6. Dir permissions umask 7. MergeWithRules wildcards — document limitation only

agent commented 2026-09-20 00:46:47 +00:00

Author

Member

Copy link

Implementation complete

All 7 fixes implemented and verified:

1. InitWithTemplate error swallowing — now returns errors from template render, mkdir, and write. Closes #1.
2. Float support — added reflect.Float32/Float64 to setFromString, coerceDefault, and coerceAndSet.
3. Stale docs — optional → omitempty in README and overview; updated integration status.
4. structToMap omitempty — nested struct fields with omitempty:"true" are now skipped when zero and no default.
5. RenderTemplate missing keys — returns error listing all undefined placeholder keys. Two-pass: validate then substitute.
6. MergeWithRules wildcards — documented limitation (leaf names only, not dotted paths).
7. Dir permissions — 0o700 → 0o777 to respect umask.

Tests added

• TestInitWithTemplate, TestInitWithTemplateBadTemplate, TestInitWithTemplateExistingFile, TestInitWithTemplateReadOnlyDir
• TestDefaultsFloat, TestLoadFloat, TestEnvFloat, TestWriteFloat, TestWriteFloatWithDefault
• TestWriteNestedOmitEmpty, TestWriteNestedOmitEmptyNonZero

Verification

• build: PASS
• tests: PASS (all existing + 11 new)
• vet: PASS

## Implementation complete All 7 fixes implemented and verified: 1. **InitWithTemplate error swallowing** — now returns errors from template render, mkdir, and write. Closes #1. 2. **Float support** — added `reflect.Float32`/`Float64` to `setFromString`, `coerceDefault`, and `coerceAndSet`. 3. **Stale docs** — `optional` → `omitempty` in README and overview; updated integration status. 4. **structToMap omitempty** — nested struct fields with `omitempty:"true"` are now skipped when zero and no default. 5. **RenderTemplate missing keys** — returns error listing all undefined placeholder keys. Two-pass: validate then substitute. 6. **MergeWithRules wildcards** — documented limitation (leaf names only, not dotted paths). 7. **Dir permissions** — `0o700` → `0o777` to respect umask. ### Tests added - `TestInitWithTemplate`, `TestInitWithTemplateBadTemplate`, `TestInitWithTemplateExistingFile`, `TestInitWithTemplateReadOnlyDir` - `TestDefaultsFloat`, `TestLoadFloat`, `TestEnvFloat`, `TestWriteFloat`, `TestWriteFloatWithDefault` - `TestWriteNestedOmitEmpty`, `TestWriteNestedOmitEmptyNonZero` ### Verification - build: PASS - tests: PASS (all existing + 11 new) - vet: PASS

agent commented 2026-09-20 15:06:03 +00:00

Author

Member

Copy link

Status Update — 2026-09-20

The original 7 items from this plan are all implemented and verified (per comment above). However, a full production-readiness audit on 2026-09-20 identified additional gaps. These are now tracked as separate issues:

New issues created

#	Title	Tier	Status
#4	go.mod: yaml.v3 marked as indirect but directly imported	tier-0-trivial	Open
#5	Add GitHub Actions CI workflow	tier-1-easy	Open
#6	Document cmd/ as internal dev tools in README	tier-0-trivial	Open
#7	Add map[string]T support for config fields	tier-2-medium	Open
#8	Add non-string slice env var support ([]int, []bool, etc.)	tier-2-medium	Open
#9	Fix InitWithTemplate mkdir permissions inconsistency (0o700 vs 0o777)	tier-0-trivial	Open
#10	Add Known Limitations section to README	tier-0-trivial	Open

Original items — completion status

Item	Status
1. InitWithTemplate error swallowing	✅ Fixed (errors returned)
2. Float support in setFromString/coerceDefault	✅ Fixed + tests added
3. Stale docs (optional → omitempty)	✅ Fixed
4. structToMap omitempty propagation	✅ Fixed + tests added
5. RenderTemplate missing key handling	✅ Fixed (returns error)
6. MergeWithRules wildcard matching	✅ Documented as limitation
7. write.go dir permissions	✅ Fixed (0o700 → 0o777)

This issue can be closed once all new issues (#4-#10) are resolved.

## Status Update — 2026-09-20 The original 7 items from this plan are **all implemented and verified** (per comment above). However, a full production-readiness audit on 2026-09-20 identified additional gaps. These are now tracked as separate issues: ### New issues created | # | Title | Tier | Status | |---|-------|------|--------| | #4 | go.mod: yaml.v3 marked as indirect but directly imported | tier-0-trivial | Open | | #5 | Add GitHub Actions CI workflow | tier-1-easy | Open | | #6 | Document cmd/ as internal dev tools in README | tier-0-trivial | Open | | #7 | Add map[string]T support for config fields | tier-2-medium | Open | | #8 | Add non-string slice env var support ([]int, []bool, etc.) | tier-2-medium | Open | | #9 | Fix InitWithTemplate mkdir permissions inconsistency (0o700 vs 0o777) | tier-0-trivial | Open | | #10 | Add Known Limitations section to README | tier-0-trivial | Open | ### Original items — completion status | Item | Status | |------|--------| | 1. InitWithTemplate error swallowing | ✅ Fixed (errors returned) | | 2. Float support in setFromString/coerceDefault | ✅ Fixed + tests added | | 3. Stale docs (optional → omitempty) | ✅ Fixed | | 4. structToMap omitempty propagation | ✅ Fixed + tests added | | 5. RenderTemplate missing key handling | ✅ Fixed (returns error) | | 6. MergeWithRules wildcard matching | ✅ Documented as limitation | | 7. write.go dir permissions | ✅ Fixed (0o700 → 0o777) | This issue can be closed once all new issues (#4-#10) are resolved.

agent referenced this issue 2026-09-20 15:33:52 +00:00

Plan: Production readiness — v0.1.0 release blockers #11

merith-tk referenced this issue from a commit 2026-09-20 15:50:26 +00:00

fix: address all 7 config library issues

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

No results found.

Labels

Clear labels   

proj-config-lib

config library (standalone)

  

tier-0-trivial

One-line fix, typo, config change — zero risk

  

tier-1-easy

Small contained change, &lt;50 LOC, single file

  

tier-2-medium

Multi-file change within one project

No labels

proj-config-lib

tier-0-trivial

tier-1-easy

tier-2-medium

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

residual/config#3

No description provided.