Conversation
- Die `goConfy` Codegenerierung wurde deterministisch gestaltet und Golden Tests wurden zur Sicherstellung der Output-Stabilität eingeführt. ### Changes - `gen/registry/registry.go`: Die Methode `List()` sortiert nun die Provider-IDs alphabetisch, um eine stabile Reihenfolge bei der Auswahl zu gewährleisten. - `tests/golden_test.go`: Ein neuer Golden Test wurde implementiert, der die Befehle `init`, `fmt` und `dump` gegen erwartete Ergebnisse prüft. - `tests/testdata/golden/`: Verzeichnis mit Referenzdateien (`.yml`, `.env`, `.json`) für die Golden Tests erstellt. - `Makefile`: Ein neues Target `update-golden` wurde hinzugefügt, um die Referenzdateien bei absichtlichen Änderungen einfach aktualisieren zu können. ### Verification - Die Golden Tests wurden erfolgreich ausgeführt und verifiziert. - Ein mehrfacher Durchlauf der Tests ohne Änderungen zeigt keine Abweichungen (Deterministik bestätigt). - OS-unabhängige Vergleiche durch Normalisierung der Zeilenenden wurden implementiert.
- Die Fehlerberichterstattung in `goConfy` wurde verbessert, um detaillierte Informationen wie Zeile, Spalte, Dot-Path und den betroffenen Layer (z.B. base, profile) bereitzustellen. ### Changes - **errors.go**: `FieldError` um die Felder `Line`, `Column`, `Layer` und `Path` erweitert, ohne die Kompatibilität zu unterbrechen. - **internal/envmacro**: `ExpandNode` verfolgt nun den Pfad während der Rekursion und gibt detaillierte Fehler mit Position und Pfad zurück. - **internal/decode**: `Strict` Decoding extrahiert nun Zeilennummern aus YAML-Fehlern für unbekannte Felder mittels Regex-Parsing. - **loader.go**: Der Loader reichert Fehler nun mit Layer-Informationen an, um die Fehlerquelle (z.B. Basis-Konfiguration oder Profile) zu identifizieren. ### Verification - Bestehende Tests wurden aktualisiert, um die neuen Fehlerdetails zu validieren. - Neue Testfälle in `tests/strict_test.go` und `tests/envmacro_test.go` verifizieren, dass Pfade (z.B. `server.host`) und Zeilennummern korrekt gemeldet werden. - Alle Tests wurden erfolgreich ausgeführt (`go test ./...`).
- Implementierung eines optionalen Explain/Trace-Reportings für Konfigurationswerte, um deren Herkunft (Basis, Profil, Umgebungsvariablen) nachvollziehbar zu machen, ohne Geheimnisse (Secrets) preiszugeben. ### Changes - **Neues Package `explain`**: Definition von `Report` und `Entry` Strukturen sowie Methoden zur Text- und JSON-Ausgabe. - **Option `WithExplainReporter`**: Ermöglicht das Registrieren einer Callback-Funktion, die den fertigen Report nach dem Ladevorgang erhält. - **Instrumentierung des `Load`-Prozesses**: Erfassung der Werte aus der Basis-YAML, nach der Expansion von Umgebungsvariablen und nach Anwendung von Profil-Overrides. - **Sicherheits-Features**: Integration mit dem `secret:"true"` Tag, um sicherzustellen, dass sensible Werte im Report immer als `[REDACTED]` erscheinen. - **Erweiterung interner Packages**: Hinzufügen von `IsSecret` im `redact` Package und eines `OnExpand` Callbacks im `envmacro` Package. ### Verification - **Automatisierte Tests**: `tests/explain_test.go` verifiziert die korrekte Erfassung von Quellen, Overrides und die zuverlässige Maskierung von Geheimnissen. - **Manueller Check**: Bestätigung, dass bei deaktivierter Option keine zusätzlichen Allokationen oder Logikschritte ausgeführt werden.
- Implementierung der "Redaction-by-Convention" (Opt-in) für automatische Geheimnis-Redigierung basierend auf Feldnamen.
- Einführung des `{FILE:/path:default}` Makros zum sicheren Laden von Geheimnissen aus Dateien.
- Konsolidierung der Makro-Logik in einem neuen, erweiterbaren System unter Beibehaltung der Rückwärtskompatibilität.
### Changes
- **Redaction**: Neues Feld `ByConvention` in `redact.Options` hinzugefügt. Felder mit Namen wie `password`, `token`, `secret`, `key` oder `private` werden automatisch redigiert, wenn die Option aktiv ist.
- **Macros**: Neues Paket `internal/macros` erstellt, das sowohl `{ENV:...}` als auch `{FILE:...}` Makros unterstützt. `{FILE:...}` liest Dateiinhalte, führt einen automatischen Trim aus und unterstützt Standardwerte.
- **Loader**: Integration in `Load()` angepasst, um `Explain`-Reports korrekt mit Makro-Quellen (`file`, `env`, `default`) zu füllen.
- **Optionen**: `WithRedactByConvention(bool)` als globale Option und `WithRedactByConventionOption(bool)` für Einzelaufrufe hinzugefügt.
- **Abwärtskompatibilität**: `internal/envmacro` zu einem Proxy-Paket für `internal/macros` umgebaut, um bestehende CLI- und TUI-Komponenten nicht zu beeinträchtigen.
- **Dokumentation**: `SECURITY_MODEL.md` und `README.md` um Sicherheitsregeln für das Dateimakro und die Konventions-Redigierung erweitert.
### Verification
- Neue Testsuite `tests/hardening_test.go` erstellt, die Redaction-by-Convention, das FILE-Makro und die korrekte Report-Generierung (Source detection) prüft.
- Bestehende `loader_test.go` und `envmacro_test.go` erfolgreich ausgeführt, um Regressionsfreiheit zu garantieren.
- Alle Tests sind passend zur neuen Logik "grün".
- Fuzz-Tests für Parser- und Transformer-Komponenten hinzugefügt, um Stabilität gegenüber zufälligen oder bösartigen Eingaben zu gewährleisten. ### Changes - `internal/macros/fuzz_test.go`: Fuzz-Test für ENV/FILE Makro-Expansion hinzugefügt. Deckt Pfade mit/ohne Standardwerte und verschiedene YAML-Strukturen ab. - `internal/dotenv/fuzz_test.go`: Fuzz-Test für den Dotenv-Parser hinzugefügt. Testet Quoting, Kommentare und fehlerhafte Zeilenformate. - `internal/profiles/fuzz_test.go`: Fuzz-Test für `DeepMerge` und `ApplyProfile` hinzugefügt. Verifiziert die Robustheit der Profil-Merging-Logik auf YAML-Knotenebene. ### Verification - Alle Fuzz-Tests wurden mit `go test -fuzz` lokal ausgeführt (jeweils 5-10 Sekunden). - Die Tests decken Panics und Hangs ab; es wurden während des Fuzzings keine Abstürze festgestellt. - Invarianten (keine Panics, definierte Fehlerzustände) wurden erfolgreich verifiziert.
- Festlegung der goConfy Best Practices und Config-Policy für eine konsistente Konfigurationsverwaltung. - Bereitstellung einer kanonischen Referenz-Konfiguration mit vollständiger Dokumentation und Validierung. ### Changes - Erstellung von `docs/CONFIG_POLICY.md` mit verbindlichen Regeln (No hidden config, Single source of truth, Pflicht-Kommentare). - Implementierung einer Referenz-Struct in `internal/config/config.go` und einer zugehörigen `config.example.yml`. - Bereitstellung eines maschinenlesbaren JSON-Schemas in `schema/config.schema.json`. - Erweiterung des Generators um den `options` Tag zur automatischen Dokumentation von Enums/erlaubten Werten. - Einführung von CI-Tests in `internal/config/config_test.go`, die die Synchronität zwischen Code, Beispiel-YAML und Schema sicherstellen. ### Verification - Alle Tests in `internal/config` erfolgreich (Strict-Loading, Reflection-Check, Schema-Check). - Alle bestehenden Tests im Root und in `tools/` erfolgreich. - Manuelle Prüfung der generierten Kommentare und der YAML-Struktur gemäß der neuen Policy.
- Repository hardening completed by removing binaries, updating ignore rules, and adding a CI guard. ### Changes - Removed committed Mach-O binaries `tools/goconfygen` and `tools/goconfytui`. - Expanded `.gitignore` with standard Go rules, build directories (`bin/`, `build/`, `dist/`), and specific binary patterns. - Added a `Binary Guard` step to `.github/workflows/ci.yml` to fail the build if binaries or executables are committed. - Updated `Makefile` to build tool binaries into `tools/bin/` (ignored) instead of the module root. - Added a "Repository Hygiene" section to `CONTRIBUTING.md` and updated `CHANGELOG.md`. ### Verification - Verified repo cleanliness using `find` for common binary extensions and executable bits. - Manually confirmed `Makefile` now uses `-o bin/` for tool builds. - Checked `.gitignore` coverage for the new build locations.
Keksclan
added
documentation
📝 WalkthroughWalkthroughThe PR refactors goConfy by moving tools (goconfygen, goconfytui) into a separate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader
participant MacroEngine
participant RedactEngine
participant ExplainReporter
Client->>Loader: Load(config, opts)
Loader->>Loader: collectBaseEntries()
Loader->>ExplainReporter: Report initialized
Loader->>MacroEngine: ExpandNode(yaml, opts)
Note over MacroEngine: Detect ENV/FILE macros
MacroEngine->>MacroEngine: Resolve macro values
MacroEngine->>ExplainReporter: OnExpand callback
Note over ExplainReporter: Record source (env/file/default)
MacroEngine-->>Loader: Expanded YAML
Loader->>Loader: collectProfileEntries()
Loader->>Loader: DeepMerge(profile overrides)
Loader->>RedactEngine: Apply(config, opts)
Note over RedactEngine: Check ByConvention flag
RedactEngine->>RedactEngine: IsConventionSecret(field_name)
Note over RedactEngine: Match against password, secret, token, key, private
RedactEngine->>ExplainReporter: Mark as secret, redact value
RedactEngine-->>Loader: Redacted config
Loader->>ExplainReporter: Finalize report
Loader-->>Client: Loaded config + Explain report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the repository and expands the config tooling by adding CI/Make targets, splitting optional tooling into a separate tools/ Go module, and extending the core loader with new macro + redaction + explain/reporting capabilities alongside broader test coverage and documentation updates.
Changes:
- Add CI workflow + Makefile “verify” pipeline (fmt/lint/test/race/vulncheck) and repository hygiene checks.
- Split generator/TUI into
tools/module and introduce/extend generator + TUI components and golden tests. - Expand core functionality (macros incl.
{FILE:...}, explain reporting, redaction-by-convention, improved errors) and add integration/fuzz/hardening tests plus config policy artifacts (example config + schema).
Reviewed changes
Copilot reviewed 77 out of 95 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci.yml | Adds CI workflow running verification and binary guard checks. |
| .gitignore | Updates ignored artifacts (binaries, coverage, workspace, IDE/OS). |
| CHANGELOG.md | Documents new features, tooling split, and hardening items. |
| CONTRIBUTING.md | Updates build instructions and repo hygiene rules. |
| Makefile | Adds unified local/CI targets (verify/test/lint/vulncheck/build). |
| README.md | Updates docs for tools split, macros, and compatibility section. |
| RELEASE.md | Adds/updates release guidance. |
| config.example.yml | Adds canonical example configuration file. |
| docs/CLI.md | Updates CLI docs for tools module paths/build instructions. |
| docs/CONFIG_POLICY.md | Adds config policy/best practices document. |
| docs/GENERATOR.md | Adds generator/TUI module documentation. |
| docs/GETTING_STARTED.md | Updates getting-started instructions to reflect tools module split. |
| docs/INSTALL_TOOLS.md | Adds guidance for core-only vs tools installation. |
| docs/SECURITY_MODEL.md | Updates security model for {FILE:...} macro + redaction-by-convention. |
| errors.go | Extends FieldError formatting/details. |
| examples/dotenv/.env | Updates example dotenv value. |
| explain/report.go | Adds explain reporting types and output helpers. |
| go.mod | Updates core module dependencies (core-only). |
| go.sum | Removes tool-only deps from core module sum. |
| internal/config/config.go | Adds canonical reference config struct with tags (desc/default/options/env). |
| internal/config/config_test.go | Adds tests to keep config.example.yml/schema in sync with struct. |
| internal/decode/strict.go | Improves strict decode errors by extracting field/line info. |
| internal/dotenv/fuzz_test.go | Adds fuzz test for dotenv parsing. |
| internal/envmacro/expand.go | Routes envmacro expansion through new macros package. |
| internal/macros/expand.go | Adds unified macro expansion for {ENV:...} and {FILE:...}. |
| internal/macros/fuzz_test.go | Adds fuzz coverage for macro expansion. |
| internal/profiles/fuzz_test.go | Adds fuzz coverage for profile deep-merge/apply behavior. |
| internal/redact/engine.go | Adds redaction-by-convention and secret detection helpers. |
| internal/redact/reflect.go | Extends redaction engine to support convention-based redaction. |
| loader.go | Adds explain reporting capture, macro expansion integration, and richer FieldError surfaces. |
| options.go | Adds options for explain reporting and redaction-by-convention. |
| redact.go | Plumbs redaction-by-convention option through Redacted(). |
| schema/config.schema.json | Adds JSON schema for the canonical example configuration. |
| tests/envmacro_test.go | Adds assertions for path/line reporting and deep-path errors. |
| tests/explain_test.go | Adds tests for explain reporting behavior and redaction. |
| tests/hardening_test.go | Adds tests for convention redaction + file macro + explain interactions. |
| tests/integration/integration_test.go | Adds black-box integration coverage of full pipeline behaviors. |
| tests/integration/testdata/.env | Adds integration test dotenv fixture. |
| tests/integration/testdata/base.yml | Adds integration test YAML fixture. |
| tests/integration/testdata/db_url.txt | Adds integration test file-macro fixture. |
| tests/loader_test.go | Updates MultiError test expectation messaging. |
| tests/strict_test.go | Tightens strict-mode tests to assert FieldError details. |
| tools/cmd/goconfygen/main.go | Updates tool entrypoint to new internal commands package path. |
| tools/cmd/goconfytui/main.go | Updates install docs and imports for tools module TUI app. |
| tools/docs/RELEASE_CHECKLIST.md | Adds release checklist for tools/core verification. |
| tools/examples/generator/.env | Adds generator example dotenv output. |
| tools/examples/generator/README.md | Updates example run instructions for tools module. |
| tools/examples/generator/config.yml | Adds generator example YAML output. |
| tools/examples/generator/register.go | Updates imports to tools module generator/registry/yamltemplate. |
| tools/examples/tui/.env | Adds TUI example dotenv file. |
| tools/examples/tui/README.md | Updates TUI example paths and run instructions for tools module. |
| tools/examples/tui/config.yml | Adds TUI sample config with macros/profiles. |
| tools/generator/registry/doc.go | Updates registry docs for new import path. |
| tools/generator/registry/registry.go | Sorts provider IDs for deterministic listing. |
| tools/generator/yamltemplate/comments.go | Adds “options” tag output to comments. |
| tools/generator/yamltemplate/defaults.go | Adds default/zero-value/macro field value helpers. |
| tools/generator/yamltemplate/doc.go | Adds package documentation. |
| tools/generator/yamltemplate/emit.go | Adds YAML + dotenv template emit implementation. |
| tools/generator/yamltemplate/reflect.go | Extends reflection metadata to include options tag. |
| tools/go.mod | Introduces separate tools module with Charm deps and core dependency. |
| tools/go.sum | Adds tools module dependency checksums. |
| tools/internal/tools/generator/commands/doc.go | Updates docs for generator package rename. |
| tools/internal/tools/generator/commands/dump.go | Updates registry import path for dump command. |
| tools/internal/tools/generator/commands/fmt.go | Adds fmt subcommand implementation with optional macro expansion. |
| tools/internal/tools/generator/commands/init.go | Updates registry/yamltemplate import paths for init command. |
| tools/internal/tools/generator/commands/paths.go | Adds shared path resolution helpers for CLI commands. |
| tools/internal/tools/generator/commands/validate.go | Updates registry import path for validate command. |
| tools/internal/tools/tui/app/model.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/app/routes.go | Adds screen routing enum for TUI. |
| tools/internal/tools/tui/components/confirm.go | Updates style import path. |
| tools/internal/tools/tui/components/footer_help.go | Updates style import path. |
| tools/internal/tools/tui/components/header.go | Updates style import path. |
| tools/internal/tools/tui/components/inputs.go | Adds text input helper. |
| tools/internal/tools/tui/components/list.go | Updates style import path. |
| tools/internal/tools/tui/components/viewport.go | Adds viewport helper. |
| tools/internal/tools/tui/logic/operations.go | Updates imports for generator packages + TUI state. |
| tools/internal/tools/tui/logic/preview.go | Adds YAML redaction + detection helpers for previews. |
| tools/internal/tools/tui/screens/dump.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/filepicker.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/fmt.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/home.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/init.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/inspect.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/settings.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/screens/validate.go | Updates imports to tools module TUI packages. |
| tools/internal/tools/tui/state/config.go | Adds shared TUI config/state + defaults. |
| tools/internal/tools/tui/style/theme.go | Adds centralized Lipgloss theme/styles. |
| tools/tests/golden_test.go | Adds golden tests for init/fmt/dump output. |
| tools/tests/registry_test.go | Updates registry import path. |
| tools/tests/tui_logic_test.go | Updates imports for moved TUI logic/state packages. |
| tools/tests/yamltemplate_test.go | Updates yamltemplate import path. |
| tools/tests/testdata/golden/dump-out.json | Adds golden output for dump command. |
| tools/tests/testdata/golden/fmt-out.yml | Adds golden output for fmt command. |
| tools/tests/testdata/golden/init-config.yml | Adds golden output for init command YAML template. |
| tools/tests/testdata/golden/init.env | Adds golden output for init-generated dotenv. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // FileMacroRegex is the regex for file macros. | ||
| // Pattern: {FILE:/path/to/file} or {FILE:/path/to/file:default} | ||
| var FileMacroRegex = regexp.MustCompile(`^\{FILE:([^:]+)(?::([^}]*))?\}$`) |
There was a problem hiding this comment.
FileMacroRegex rejects any file path containing :. This breaks {FILE:...} on Windows drive paths (e.g., C:\...) and other colon-containing paths. Consider relaxing the regex (or switching to a small parser) so the delimiter is the first colon after FILE: and everything up to the optional :default is treated as the path.
Suggested change
| var FileMacroRegex = regexp.MustCompile(`^\{FILE:([^:]+)(?::([^}]*))?\}$`) | |
| var FileMacroRegex = regexp.MustCompile(`^\{FILE:([^}:][^}]*)(?::([^}]*))?\}$`) |
|
|
||
| ## update-golden: Update golden files for tests | ||
| update-golden: | ||
| go test ./tests -run TestGolden -update |
There was a problem hiding this comment.
update-golden runs go test ./tests -run TestGolden -update, but TestGolden is in tools/tests (not ./tests). As written, this target won’t update the golden files. Point the command at the correct package (e.g., cd tools && go test ./tests -run TestGolden -update).
Suggested change
| go test ./tests -run TestGolden -update | |
| cd tools && go test ./tests -run TestGolden -update |
Comment on lines
+45
to
+53
| case yaml.SequenceNode: | ||
| for i, child := range node.Content { | ||
| newPath := fmt.Sprintf("%s[%d]", path, i) | ||
| collectBaseEntries[T](child, report, newPath, opts) | ||
| } | ||
| case yaml.ScalarNode: | ||
| isSecret := redact.IsSecret(reflect.TypeFor[T](), path, opts) | ||
| report.AddEntry(path, explain.SourceBase, node.Value, isSecret, "") | ||
| } |
There was a problem hiding this comment.
collectBaseEntries uses paths like field[0] for sequence elements, but redact.IsSecret only understands dot-separated struct paths. This means slice/array elements of secret fields won’t be recognized as secret and can leak into the explain report unredacted. Consider keeping the structural path (e.g., field) for secret detection/redaction, or teaching IsSecret/path handling to ignore [...] indices.
Comment on lines
+5
to
+11
| require ( | ||
| github.com/charmbracelet/bubbles v1.0.0 | ||
| github.com/charmbracelet/bubbletea v1.3.10 | ||
| github.com/charmbracelet/lipgloss v1.1.0 | ||
| github.com/keksclan/goConfy v0.0.0-20260219103719-a3b470266ef3 | ||
| gopkg.in/yaml.v3 v3.0.1 | ||
| ) |
There was a problem hiding this comment.
Because tools is a separate module, cd tools && go test ./... will resolve github.com/keksclan/goConfy from the network at the pinned pseudo-version, not from the checked-out code in this PR. That means CI/local make test may test tools against a different core than the one being changed here. Consider committing a go.work that uses both . and ./tools (or another workspace/replace strategy) so the tools module is built against the local core during repository builds.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (14)
tools/generator/yamltemplate/reflect.go (1)
22-23: Normalizeoptionsinto a structured type at extraction time.Line 22–Line 23 describe a list, but Line 82 stores raw tag text. Consider parsing once into
[]stringto avoid repeated splitting/trim logic downstream.♻️ Proposed refactor
type fieldInfo struct { - // Options is the list of allowed values from `options:"..."` tag. - Options string + // Options is the list of allowed values from `options:"..."` tag. + Options []string } @@ - info.Options = f.Tag.Get("options") + if raw := f.Tag.Get("options"); raw != "" { + parts := strings.Split(raw, ",") + opts := make([]string, 0, len(parts)) + for _, p := range parts { + p = strings.TrimSpace(p) + if p != "" { + opts = append(opts, p) + } + } + info.Options = opts + }Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/generator/yamltemplate/reflect.go` around lines 22 - 23, Change the Options field from a raw string to a structured []string and parse the tag once at extraction time: update the struct field declaration (Options string → Options []string) and in the code that currently stores the raw tag text (the assignment that sets the `options` tag at the extraction site around the current line 82) split and trim the tag value into a []string there, store that slice in Options, and update downstream consumers to use the parsed []string instead of splitting the raw text repeatedly.redact.go (1)
19-20: Optional: update API doc to mention convention-based redaction.Line 20 adds
ByConvention, but theRedacteddoc comment still describes only tag/path redaction. Consider updating it to reflect the new behavior for clarity.Suggested doc tweak
-// Fields tagged with `secret:"true"` and any explicitly configured paths are -// replaced with "[REDACTED]". +// Fields tagged with `secret:"true"`, explicitly configured paths, and +// convention-detected secret keys (when enabled) are replaced with "[REDACTED]".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@redact.go` around lines 19 - 20, The Redacted struct's doc comment only mentions tag/path redaction but the code now sets ByConvention (rc.byConvention), so update the Redacted documentation to mention convention-based redaction as well; locate the Redacted type comment and add a short sentence explaining that redaction can occur by tags, explicit paths (rc.paths) or by convention-based rules (rc.byConvention), and note any behavior differences or precedence between methods if applicable.RELEASE.md (1)
31-32: Optional: Consider German quotation marks for consistency.For formal German documentation, consider using German-style quotation marks (
„Releases"and„Draft a new release") instead of ASCII quotes. However, ASCII quotes are commonly accepted in technical documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RELEASE.md` around lines 31 - 32, Replace the ASCII double quotes around the menu labels in the two release instructions with German-style quotation marks: change "Releases" to „Releases“ and "Draft a new release" to „Draft a new release“ so the two lines (Navigiere zu "Releases" im GitHub-Repository. and Wähle "Draft a new release".) use German quotation marks for consistency in the German documentation.docs/CONFIG_POLICY.md (1)
33-33: Nitpick: German spelling of "Macros".In German text, "Makros" is the standard spelling. However, "Macros" is acceptable as a technical anglicism in software documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CONFIG_POLICY.md` at line 33, Replace the English plural "Macros" with the correct German spelling "Makros" in the sentence that contains examples like `{ENV:VAR_NAME}` and `{FILE:path}`; update the phrase so it reads e.g. "Wenn Makros wie `{ENV:VAR_NAME}` oder `{FILE:path}` verwendet werden..." and scan the same file for any other occurrences of "Macros" (including the example `{ENV:PORT:8080}` line) and change them to "Makros" for consistent German spelling.internal/macros/expand.go (2)
77-87: Consider extracting duplicated colon-counting logic into a helper function.The logic to determine
hasDefaultby counting colons is duplicated for both FILE and ENV macro handling. A small helper would improve maintainability.♻️ Suggested refactor
// hasDefaultValue returns true if the macro string contains a default value // (indicated by 2+ colons in the pattern). func hasDefaultValue(macroStr string) bool { colons := 0 for _, ch := range macroStr { if ch == ':' { colons++ } } return colons >= 2 }Then use:
hasDefault := hasDefaultValue(matches[0])in both branches.Also applies to: 122-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/macros/expand.go` around lines 77 - 87, The colon-counting logic used to set hasDefault (currently duplicated where FileMacroRegex and the ENV macro branch parse matches[0]) should be extracted into a small helper like hasDefaultValue(macroStr string) bool and then called from both places (replace the inlined loop that computes colons and sets hasDefault with hasDefault := hasDefaultValue(matches[0])). Update references in internal/macros/expand.go for the FILE handling (where FileMacroRegex is used) and the ENV handling (the other branch around lines 122-128) to call the new helper so the duplication is removed and behavior remains identical.
89-106: Consider adding optional restrictions to the FILE macro for defense-in-depth.The FILE macro accepts arbitrary file paths from YAML without validation. While the security model relies on process filesystem permissions, industry best practices recommend additional safeguards:
- Allowlist of permitted directories: The ENV macro already restricts access via
AllowedKeys; an equivalent mechanism for FILE paths would prevent unintended file access.- Symlink resolution: Canonicalize paths to prevent symlink-based directory escape attacks (CWE-22 path traversal).
- Maximum file size limit: Prevent memory exhaustion DoS by capping file reads; currently the entire file is loaded into memory without bounds.
These are optional improvements for defense-in-depth, not defects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/macros/expand.go` around lines 89 - 106, The FILE macro currently reads filePath directly into memory; add optional defense-in-depth checks before reading: 1) introduce and respect an allowlist option (e.g., add AllowedDirs or AllowedPaths to the existing opts struct) and verify the canonicalized path is under one of those allowed directories; 2) canonicalize filePath using filepath.Abs + filepath.EvalSymlinks (or equivalent) to prevent symlink/path-traversal escapes; 3) stat the resolved path and enforce a configurable MaxFileSize limit (return the same hasDefault fallback or a FieldError if too large) before calling os.ReadFile; keep existing behavior around hasDefault, node.Value and opts.OnExpand but perform these validations first and return appropriate FieldError messages when checks fail.docs/SECURITY_MODEL.md (1)
15-21: Add language specifier to fenced code block.The code block displaying regex patterns lacks a language specifier, which aids readability and tooling.
📝 Suggested fix
-``` +```text # ENV ^\{ENV:([A-Z0-9_]+)(?::([^}]*))?\}$ # FILE ^\{FILE:([^:]+)(?::([^}]*))?\}$</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/SECURITY_MODEL.mdaround lines 15 - 21, The fenced code block
containing the regex patterns for "# ENV" and "# FILE" should include a language
specifier to improve rendering and tooling; update the opening backticks of that
block to include a language (e.g., changetotext) so the block is
annotated as text while keeping the existing content for the ENV and FILE regex
examples.</details> </blockquote></details> <details> <summary>explain/report.go (1)</summary><blockquote> `92-108`: **Fixed column widths may truncate long paths or values.** The `WriteText` method uses fixed column widths (40, 10, 20, 20 characters). Long configuration paths or values will be truncated or cause misaligned output. Consider using a tabwriter for dynamic column sizing, or documenting this limitation. <details> <summary>♻️ Suggested improvement using tabwriter</summary> ```go import "text/tabwriter" func (r Report) WriteText(w io.Writer) error { tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) fmt.Fprintf(tw, "PATH\tSOURCE\tVALUE (REDACTED)\tOVERRIDES\tNOTES\n") for _, e := range r.Entries { overrides := strings.Join(sourcesToStrings(e.OverriddenBy), ", ") fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\n", e.Path, e.Source, e.ValueRedacted, overrides, e.Notes) } return tw.Flush() } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@explain/report.go` around lines 92 - 108, The WriteText method in Report currently uses fixed column widths which truncate/misaligned long fields; update Report.WriteText to use text/tabwriter (create a tabwriter.NewWriter, write headers with '\t' separators, write each entry using e.Path, e.Source, e.ValueRedacted, the joined e.OverriddenBy sources, and e.Notes separated by tabs) and call tw.Flush() at the end so columns size dynamically; reference the Report.WriteText function and the e.OverriddenBy handling (replace the fixed fmt.Fprintf format string and the overrides assembly with tab-delimited output and a final tw.Flush()). ``` </details> </blockquote></details> <details> <summary>tools/tests/golden_test.go (1)</summary><blockquote> `76-94`: **Consider using `t.Cleanup` to ensure `os.Stdout` is restored even on panic.** If `commands.RunDump` panics, `os.Stdout` will remain pointing to the pipe, potentially affecting subsequent tests. Using `t.Cleanup` ensures restoration regardless of how the test exits. <details> <summary>♻️ Suggested improvement</summary> ```diff // Capture stdout oldStdout := os.Stdout r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } os.Stdout = w + t.Cleanup(func() { os.Stdout = oldStdout }) args := []string{"-id", "test-config", "-in", inPath} err = commands.RunDump(args) w.Close() - os.Stdout = oldStdout ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/tests/golden_test.go` around lines 76 - 94, The test temporarily replaces os.Stdout (oldStdout) with a pipe (r, w) around the call to commands.RunDump; if RunDump panics the restore won't run. Use t.Cleanup to always restore os.Stdout and close the writer/reader: save oldStdout, set os.Stdout = w, then call t.Cleanup(func(){ os.Stdout = oldStdout; w.Close(); r.Close() }) before invoking commands.RunDump (referencing variables oldStdout, r, w and function commands.RunDump) so stdout is restored and the pipe closed even on panic. ``` </details> </blockquote></details> <details> <summary>internal/macros/fuzz_test.go (1)</summary><blockquote> `10-25`: **Consider adding FILE macro path edge cases to fuzz seeds.** The seeds cover ENV macro patterns well but could benefit from more FILE macro edge cases to improve coverage. <details> <summary>📝 Suggested additions</summary> ```diff "{FILE:path:with:colons}", "{INVALID:MACRO}", "{}", "{:}", "{{ENV:VAR}}", + "{FILE:}", + "{FILE::default}", + "{FILE:../relative/path}", + "{FILE:/path/with spaces}", + "{FILE:/path\nwith\nnewlines}", } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/macros/fuzz_test.go` around lines 10 - 25, The fuzz seed list in fuzz_test.go (variable seeds) lacks FILE macro path edge cases; update the seeds slice to include additional FILE patterns such as empty path "{FILE:}", absolute root "{FILE:/}", relative paths "{FILE:./path}", tilde/home "{FILE:~/file}", paths with spaces "{FILE:/tmp/my file}", paths with dots "{FILE:./.hidden}", paths containing colons and defaults "{FILE:/path:default}" and "{FILE::default}", nested/braced variants "{{FILE:/tmp}}", and invalid names like "{FILE:INVALID/..}" so the FILE macro handling (seeds) covers empty, special chars, whitespace, tilde, relative/absolute, colon-in-path and malformed cases. ``` </details> </blockquote></details> <details> <summary>tests/explain_test.go (1)</summary><blockquote> `101-111`: **Rename this test to match what it validates.** `TestExplainDisabled_NoAllocations` currently checks “works without reporter,” not allocations. A clearer name avoids false expectations. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/explain_test.go` around lines 101 - 111, The test name TestExplainDisabled_NoAllocations is misleading; rename the test function to reflect that it verifies loading without a reporter (for example TestExplainLoadWithoutReporter or TestExplainDisabled_NoReporter) by changing the function identifier, updating the test declaration and any references (e.g., call sites or comments) to the new name so the test name matches its behavior in the file's TestExplainDisabled_NoAllocations block where goconfy.Load[ExplainConfig] is invoked. ``` </details> </blockquote></details> <details> <summary>internal/config/config_test.go (2)</summary><blockquote> `123-127`: **This placeholder test should be explicitly skipped until implemented.** Right now it always passes and can suggest coverage that does not exist. Would you like me to draft a read-only scanner test that reports `os.Getenv(...)` usages not represented in the canonical config model? <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 123 - 127, The TestNoHiddenConfig placeholder always passes and should be explicitly skipped until implemented: inside the TestNoHiddenConfig function add a call to t.Skip (e.g., t.Skip("skipping placeholder test; implement scanner to verify os.Getenv usages")) as the first statement and update the comment to indicate it's intentionally skipped; this ensures the test runner marks it as skipped rather than passing and inflating coverage. ``` </details> --- `117-117`: **Use a type-safe check instead of string-matching `types.Duration`.** Comparing `field.Type.String()` is brittle; prefer `reflect.Type` equality. <details> <summary>🔧 Proposed fix</summary> ```diff import ( "encoding/json" "os" "reflect" "testing" "github.com/keksclan/goConfy" + "github.com/keksclan/goConfy/types" "gopkg.in/yaml.v3" ) @@ func checkFields(t *testing.T, rt reflect.Type, node *yaml.Node, path string) { + durationType := reflect.TypeFor[types.Duration]() + @@ - if field.Type.Kind() == reflect.Struct && field.Type.String() != "types.Duration" { + if field.Type.Kind() == reflect.Struct && field.Type != durationType { checkFields(t, field.Type, valNode, fieldPath) } } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` at line 117, The code uses a brittle string comparison field.Type.String() != "types.Duration" to detect types.Duration; instead obtain a reflect.Type for types.Duration (e.g., durationType := reflect.TypeOf(types.Duration(0))) and replace the string check with a type-safe equality check (field.Type != durationType) in the conditional that currently reads if field.Type.Kind() == reflect.Struct && field.Type.String() != "types.Duration". This ensures you compare reflect.Type values directly rather than matching on the string representation. ``` </details> </blockquote></details> <details> <summary>tests/integration/integration_test.go (1)</summary><blockquote> `181-192`: **Use a test-scoped temp file instead of a fixed `testdata/temp.env` path** Using a fixed file path here can collide with concurrent runs and is less failure-safe than `t.TempDir()`. <details> <summary>♻️ Proposed fix</summary> ```diff +import "path/filepath" ``` ```diff - // Create a temp .env file - dotenv := "ONLY_IN_DOTENV=secret-val" - err := os.WriteFile("testdata/temp.env", []byte(dotenv), 0644) + // Create a temp .env file + dotenv := "ONLY_IN_DOTENV=secret-val" + dotenvPath := filepath.Join(t.TempDir(), ".env") + err := os.WriteFile(dotenvPath, []byte(dotenv), 0600) if err != nil { t.Fatal(err) } - defer os.Remove("testdata/temp.env") _, err = goconfy.Load[FullConfig]( goconfy.WithBytes([]byte(yaml)), - goconfy.WithDotEnvFile("testdata/temp.env"), + goconfy.WithDotEnvFile(dotenvPath), ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/integration/integration_test.go` around lines 181 - 192, Replace the fixed "testdata/temp.env" with a test-scoped temporary file created under t.TempDir(), write the dotenv contents to that path, and pass that path to goconfy.WithDotEnvFile so tests don't collide; use t.TempDir() and t.Cleanup (or defer os.Remove) to ensure cleanup and update the write/read calls around os.WriteFile and goconfy.Load accordingly (referencing the test function in integration_test.go and the goconfy.Load / goconfy.WithDotEnvFile usage). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 44-47: The CI step that installs golangci-lint currently pipes the
mutable master install script and downloads v1.64.5 (built with Go 1.24), which
is incompatible with Go 1.26; replace that step to build/install golangci-lint
with the current Go toolchain using go install
github.com/golangci/golangci-lint/cmd/golangci-lint@ (or
@latest) instead of the curl installer, and ensure the runner's PATH includes
$(go env GOPATH)/bin or set GOBIN so the installed golangci-lint binary is
available to subsequent steps.In
@CHANGELOG.md:
- Line 13: The CHANGELOG.md entry references docs/RELEASE_CHECKLIST.md but the
actual document is named RELEASE.md; update the release checklist line in
CHANGELOG.md to reference the correct path/name (e.g., RELEASE.md or
docs/RELEASE.md to match the repo), ensuring the text and link point to the
repository’s actual release document name used elsewhere in the PR.In
@errors.go:
- Around line 32-34: The current error message always prints "col 0" when Line >
0 but Column is 0; update the conditional around the
sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column)) so that
column is only included when e.Column > 0 (e.g., print "line N: " if Column is
zero and "line N, col M: " when Column > 0), locating the change near the code
referencing e.Line and e.Column and the sb.WriteString call.In
@examples/dotenv/.env:
- Line 4: Move the APP_VERSION_DISPLAY entry so dotenv keys are in the expected
order: place the APP_VERSION_DISPLAY key above the GRPC_PORT key in the .env
file to resolve the UnorderedKey linter warning (reorder the APP_VERSION_DISPLAY
and GRPC_PORT entries so APP_VERSION_DISPLAY appears first).In
@internal/decode/strict.go:
- Around line 20-22: The current return of just the parsed wrapper from
wrapError(err) discards the original decode error; update the branches that
currently do "if wrapped, ok := wrapError(err) { return wrapped }" (including
the other occurrences in the 28-40 region) to return a combined error that
preserves both the field-structured wrapper and the original err (e.g., by
joining or wrapping them so both are in the chain). Locate wrapError and the
code paths that produce fieldError and ensure the returned error includes both
the wrapped value and the original err (use errors.Join or an appropriate
wrapping pattern) so callers receive the field metadata plus full decode
diagnostics.In
@internal/envmacro/expand.go:
- Around line 16-25: The docstring for ExpandNode is inaccurate: it says it only
expands {ENV:...} macros but the implementation calls macros.ExpandNode which
also expands {FILE:...} macros. Update the docstring of ExpandNode to accurately
describe that it expands environment and file macros (or both ENV and FILE
patterns) and list any relevant options from ExpandOptions (Lookup, Prefix,
AllowedKeys, OnExpand) that influence behavior; alternatively, if you need
ENV-only behavior, add a new function (e.g., ExpandEnvNode) that calls a macros
method configured to only expand ENV and keep ExpandNode delegating to macros.
Ensure references to macros.ExpandNode and ExpandOptions are mentioned so
reviewers can find and change the implementation or add the ENV-only wrapper.In
@internal/redact/engine.go:
- Around line 71-81: The current IsConventionSecret implementation uses
strings.Contains against conventionKeywords (var conventionKeywords) and the
broad keyword "key" causes false positives (e.g., primaryKey, keyboardLayout).
Update the matching logic: either remove "key" from conventionKeywords and add
more specific entries like "apikey", "sshkey", "secret_key", or replace the
simple substring check in IsConventionSecret with a stricter match (e.g., regex
word-boundary/camelCase/snake_case-aware patterns) so only whole words or known
secret patterns are flagged; then add unit tests covering false positives
(primaryKey, keyboardLayout, lookupKey) and true positives (apiKey, secret_key)
to validate the change.In
@loader.go:
- Around line 108-113: collectOverrides currently handles yaml.MappingNode and
yaml.ScalarNode but ignores yaml.SequenceNode, so list-field overrides never get
reported; update collectOverrides to detect yaml.SequenceNode (in the same
branch structure where MappingNode and ScalarNode are handled) and iterate its
Elements, calling collectOverrides for nested mapping/sequence elements or
recording scalar items via report.AddEntry; when recording sequence items
include an index in the path (e.g., append "[i]" or similar to newPath) so each
list element override is reported, and reuse
redact.IsSecret(reflect.TypeForT, newPath, opts) for scalar secrecy checks.In
@Makefile:
- Line 1: The .PHONY declaration currently omits some non-file targets; update
the Makefile's .PHONY line (which currently lists test test-race lint fmt-check
vulncheck verify help) to also include the targets all, build-tools, and
update-golden so they are treated as phony targets and won't silently become
no-ops if files with those names exist; modify the .PHONY entry to include those
three names alongside the existing ones.- Around line 40-41: Replace the non-deterministic use of
@latestin the
Makefile's govulncheck invocations by pinning to a stable version (e.g., change
occurrences of "go run golang.org/x/vuln/cmd/govulncheck@latest ./..." in the
Makefile to use the release tag "@v1.1.4"); update both the top-level and the cd
tools invocation so both use the pinned version to ensure reproducible CI.In
@README.md:
- Around line 33-39: The Versioning subsection in README.md is written in German
while the rest of the document is English; update the subsection text (the lines
containing "Wir folgen [SemVer]" and the bullet points for v0.x.x and v1.x.x) to
English so the entire README remains consistent—e.g., replace "Wir folgen
[SemVer]" with "We follow [SemVer]" and translate the two bullets about
experimental phase and stable API/compatibility into clear English while
preserving the links and meaning.In
@schema/config.schema.json:
- Line 76: The duration regex for db.timeout and redis.ttl only accepts a single
number+unit token; update the pattern so it accepts one or more consecutive
Go-style number+unit groups (e.g. "1h30m", "100ms"). Replace the existing
pattern string (currently "^[0-9]+(s|m|h|ms|us|ns)$") with a pattern that allows
repeated groups and lists multi-character units before single-character ones
(for example a pattern matching ^([0-9]+(ns|us|ms|h|m|s))+$), and apply this
change for both the db.timeout and redis.ttl entries in the schema.In
@tests/hardening_test.go:
- Around line 68-71: The test constructs YAML using double-quoted values with a
raw file path macro ("{FILE:PATH}") which breaks on Windows backslashes; update
the YAML snippets built where the variable yaml is created (and the similar
block around the other occurrence) to use single-quoted FILE macros (e.g.
'{FILE:PATH}') and normalize the inserted secretFile slashes (replace
backslashes with forward slashes before calling strings.ReplaceAll) so the
replacement is YAML-safe and cross-platform; make the same change for the second
occurrence referenced in the comment.In
@tests/integration/integration_test.go:
- Around line 259-268: The test currently only validates properties for entries
if they exist; update the logic around capturedReport.Entries to explicitly
assert that entries with Path "db.password" and "secret_key" are present: track
booleans (e.g., foundDBPassword, foundSecretKey) while iterating over
capturedReport.Entries and perform the IsSecret and ValueRedacted checks as
already written, then after the loop add t.Errorf/t.Fatalf assertions if
foundDBPassword or foundSecretKey are false so the test fails when those paths
are missing.- Around line 117-120: Replace direct process-env mutations (os.Unsetenv) in the
"MissingRequiredEnvMacro" subtest with the scoped testing helper t.Setenv to
avoid leaking state; specifically remove calls to os.Unsetenv("DB_PASSWORD") and
use t.Setenv("DB_PASSWORD", "") within the subtest so the original value is
restored automatically, and do the same for the other occurrence that currently
unsets env in the same file; search for the subtest name
"MissingRequiredEnvMacro" and any other places where os.Unsetenv is used (the
file already uses t.Setenv elsewhere) and convert those to t.Setenv to ensure
test isolation.In
@tests/strict_test.go:
- Around line 64-87: TestErrorLayer is flaky because it relies on MISSING_VAR
not being set in the environment; ensure the test explicitly controls that
precondition by saving the current value of "MISSING_VAR", unsetting it (or
setting it to empty) before calling goconfy.Load[StrictConfig], and deferring
restoration of the original value afterward; this change should be made inside
TestErrorLayer around the call to goconfy.Load so the FieldError check
(fe.Layer/fe.Path) is deterministic regardless of runner environment.In
@tools/go.mod:
- Line 9: tools/go.mod depends on the pseudo-versioned module
github.com/keksclan/goConfy which causes local tests to use the module cache;
add a replace directive in tools/go.mod to point github.com/keksclan/goConfy to
your local checkout (a relative path to the local goConfy module) so local
changes are used during cd tools && go test ./... and then run go mod tidy;
ensure the replace targets the exact module name "github.com/keksclan/goConfy"
in the tools/go.mod.In
@tools/tests/golden_test.go:
- Around line 77-79: The call to os.Pipe() in the test ignores its error and can
yield nil r/w; update the golden_test.go block around oldStdout, r, w to capture
the error (r, w, err := os.Pipe()), check err and fail the test (e.g., t.Fatalf)
if non-nil, and ensure os.Stdout is only set to w after a successful pipe
creation; also keep the existing restore logic for oldStdout and close r/w in
defers to avoid nil dereference.
Nitpick comments:
In@docs/CONFIG_POLICY.md:
- Line 33: Replace the English plural "Macros" with the correct German spelling
"Makros" in the sentence that contains examples like{ENV:VAR_NAME}and
{FILE:path}; update the phrase so it reads e.g. "Wenn Makros wie
{ENV:VAR_NAME}oder{FILE:path}verwendet werden..." and scan the same file
for any other occurrences of "Macros" (including the example{ENV:PORT:8080}
line) and change them to "Makros" for consistent German spelling.In
@docs/SECURITY_MODEL.md:
- Around line 15-21: The fenced code block containing the regex patterns for "#
ENV" and "# FILE" should include a language specifier to improve rendering and
tooling; update the opening backticks of that block to include a language (e.g.,
changetotext) so the block is annotated as text while keeping the
existing content for the ENV and FILE regex examples.In
@explain/report.go:
- Around line 92-108: The WriteText method in Report currently uses fixed column
widths which truncate/misaligned long fields; update Report.WriteText to use
text/tabwriter (create a tabwriter.NewWriter, write headers with '\t'
separators, write each entry using e.Path, e.Source, e.ValueRedacted, the joined
e.OverriddenBy sources, and e.Notes separated by tabs) and call tw.Flush() at
the end so columns size dynamically; reference the Report.WriteText function and
the e.OverriddenBy handling (replace the fixed fmt.Fprintf format string and the
overrides assembly with tab-delimited output and a final tw.Flush()).In
@internal/config/config_test.go:
- Around line 123-127: The TestNoHiddenConfig placeholder always passes and
should be explicitly skipped until implemented: inside the TestNoHiddenConfig
function add a call to t.Skip (e.g., t.Skip("skipping placeholder test;
implement scanner to verify os.Getenv usages")) as the first statement and
update the comment to indicate it's intentionally skipped; this ensures the test
runner marks it as skipped rather than passing and inflating coverage.- Line 117: The code uses a brittle string comparison field.Type.String() !=
"types.Duration" to detect types.Duration; instead obtain a reflect.Type for
types.Duration (e.g., durationType := reflect.TypeOf(types.Duration(0))) and
replace the string check with a type-safe equality check (field.Type !=
durationType) in the conditional that currently reads if field.Type.Kind() ==
reflect.Struct && field.Type.String() != "types.Duration". This ensures you
compare reflect.Type values directly rather than matching on the string
representation.In
@internal/macros/expand.go:
- Around line 77-87: The colon-counting logic used to set hasDefault (currently
duplicated where FileMacroRegex and the ENV macro branch parse matches[0])
should be extracted into a small helper like hasDefaultValue(macroStr string)
bool and then called from both places (replace the inlined loop that computes
colons and sets hasDefault with hasDefault := hasDefaultValue(matches[0])).
Update references in internal/macros/expand.go for the FILE handling (where
FileMacroRegex is used) and the ENV handling (the other branch around lines
122-128) to call the new helper so the duplication is removed and behavior
remains identical.- Around line 89-106: The FILE macro currently reads filePath directly into
memory; add optional defense-in-depth checks before reading: 1) introduce and
respect an allowlist option (e.g., add AllowedDirs or AllowedPaths to the
existing opts struct) and verify the canonicalized path is under one of those
allowed directories; 2) canonicalize filePath using filepath.Abs +
filepath.EvalSymlinks (or equivalent) to prevent symlink/path-traversal escapes;
- stat the resolved path and enforce a configurable MaxFileSize limit (return
the same hasDefault fallback or a FieldError if too large) before calling
os.ReadFile; keep existing behavior around hasDefault, node.Value and
opts.OnExpand but perform these validations first and return appropriate
FieldError messages when checks fail.In
@internal/macros/fuzz_test.go:
- Around line 10-25: The fuzz seed list in fuzz_test.go (variable seeds) lacks
FILE macro path edge cases; update the seeds slice to include additional FILE
patterns such as empty path "{FILE:}", absolute root "{FILE:/}", relative paths
"{FILE:./path}", tilde/home "{FILE:~/file}", paths with spaces "{FILE:/tmp/my
file}", paths with dots "{FILE:./.hidden}", paths containing colons and defaults
"{FILE:/path:default}" and "{FILE::default}", nested/braced variants
"{{FILE:/tmp}}", and invalid names like "{FILE:INVALID/..}" so the FILE macro
handling (seeds) covers empty, special chars, whitespace, tilde,
relative/absolute, colon-in-path and malformed cases.In
@redact.go:
- Around line 19-20: The Redacted struct's doc comment only mentions tag/path
redaction but the code now sets ByConvention (rc.byConvention), so update the
Redacted documentation to mention convention-based redaction as well; locate the
Redacted type comment and add a short sentence explaining that redaction can
occur by tags, explicit paths (rc.paths) or by convention-based rules
(rc.byConvention), and note any behavior differences or precedence between
methods if applicable.In
@RELEASE.md:
- Around line 31-32: Replace the ASCII double quotes around the menu labels in
the two release instructions with German-style quotation marks: change
"Releases" to „Releases“ and "Draft a new release" to „Draft a new release“ so
the two lines (Navigiere zu "Releases" im GitHub-Repository. and Wähle "Draft a
new release".) use German quotation marks for consistency in the German
documentation.In
@tests/explain_test.go:
- Around line 101-111: The test name TestExplainDisabled_NoAllocations is
misleading; rename the test function to reflect that it verifies loading without
a reporter (for example TestExplainLoadWithoutReporter or
TestExplainDisabled_NoReporter) by changing the function identifier, updating
the test declaration and any references (e.g., call sites or comments) to the
new name so the test name matches its behavior in the file's
TestExplainDisabled_NoAllocations block where goconfy.Load[ExplainConfig] is
invoked.In
@tests/integration/integration_test.go:
- Around line 181-192: Replace the fixed "testdata/temp.env" with a test-scoped
temporary file created under t.TempDir(), write the dotenv contents to that
path, and pass that path to goconfy.WithDotEnvFile so tests don't collide; use
t.TempDir() and t.Cleanup (or defer os.Remove) to ensure cleanup and update the
write/read calls around os.WriteFile and goconfy.Load accordingly (referencing
the test function in integration_test.go and the goconfy.Load /
goconfy.WithDotEnvFile usage).In
@tools/generator/yamltemplate/reflect.go:
- Around line 22-23: Change the Options field from a raw string to a structured
[]string and parse the tag once at extraction time: update the struct field
declaration (Options string → Options []string) and in the code that currently
stores the raw tag text (the assignment that sets theoptionstag at the
extraction site around the current line 82) split and trim the tag value into a
[]string there, store that slice in Options, and update downstream consumers to
use the parsed []string instead of splitting the raw text repeatedly.In
@tools/tests/golden_test.go:
- Around line 76-94: The test temporarily replaces os.Stdout (oldStdout) with a
pipe (r, w) around the call to commands.RunDump; if RunDump panics the restore
won't run. Use t.Cleanup to always restore os.Stdout and close the
writer/reader: save oldStdout, set os.Stdout = w, then call t.Cleanup(func(){
os.Stdout = oldStdout; w.Close(); r.Close() }) before invoking commands.RunDump
(referencing variables oldStdout, r, w and function commands.RunDump) so stdout
is restored and the pipe closed even on panic.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `cbefbf63-de75-485d-a485-562998dbc4c9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c5ff4088b917d6139c03894039966b0b9e3b2b0b and cff6d0bfda22f388db1cda1e41182c2194bd2c92. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `go.sum` is excluded by `!**/*.sum` * `tools/go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (93)</summary> * `.github/workflows/ci.yml` * `.gitignore` * `CHANGELOG.md` * `CONTRIBUTING.md` * `Makefile` * `README.md` * `RELEASE.md` * `config.example.yml` * `docs/CLI.md` * `docs/CONFIG_POLICY.md` * `docs/GENERATOR.md` * `docs/GETTING_STARTED.md` * `docs/INSTALL_TOOLS.md` * `docs/SECURITY_MODEL.md` * `errors.go` * `examples/dotenv/.env` * `explain/report.go` * `go.mod` * `internal/config/config.go` * `internal/config/config_test.go` * `internal/decode/strict.go` * `internal/dotenv/fuzz_test.go` * `internal/envmacro/expand.go` * `internal/macros/expand.go` * `internal/macros/fuzz_test.go` * `internal/profiles/fuzz_test.go` * `internal/redact/engine.go` * `internal/redact/reflect.go` * `loader.go` * `options.go` * `redact.go` * `schema/config.schema.json` * `tests/envmacro_test.go` * `tests/explain_test.go` * `tests/hardening_test.go` * `tests/integration/integration_test.go` * `tests/integration/testdata/.env` * `tests/integration/testdata/base.yml` * `tests/integration/testdata/db_url.txt` * `tests/loader_test.go` * `tests/strict_test.go` * `tools/cmd/goconfygen/main.go` * `tools/cmd/goconfytui/main.go` * `tools/docs/RELEASE_CHECKLIST.md` * `tools/examples/generator/.env` * `tools/examples/generator/README.md` * `tools/examples/generator/config.yml` * `tools/examples/generator/register.go` * `tools/examples/tui/.env` * `tools/examples/tui/README.md` * `tools/examples/tui/config.yml` * `tools/generator/registry/doc.go` * `tools/generator/registry/registry.go` * `tools/generator/yamltemplate/comments.go` * `tools/generator/yamltemplate/defaults.go` * `tools/generator/yamltemplate/doc.go` * `tools/generator/yamltemplate/emit.go` * `tools/generator/yamltemplate/reflect.go` * `tools/go.mod` * `tools/internal/tools/generator/commands/doc.go` * `tools/internal/tools/generator/commands/dump.go` * `tools/internal/tools/generator/commands/fmt.go` * `tools/internal/tools/generator/commands/init.go` * `tools/internal/tools/generator/commands/paths.go` * `tools/internal/tools/generator/commands/validate.go` * `tools/internal/tools/tui/app/model.go` * `tools/internal/tools/tui/app/routes.go` * `tools/internal/tools/tui/components/confirm.go` * `tools/internal/tools/tui/components/footer_help.go` * `tools/internal/tools/tui/components/header.go` * `tools/internal/tools/tui/components/inputs.go` * `tools/internal/tools/tui/components/list.go` * `tools/internal/tools/tui/components/viewport.go` * `tools/internal/tools/tui/logic/operations.go` * `tools/internal/tools/tui/logic/preview.go` * `tools/internal/tools/tui/screens/dump.go` * `tools/internal/tools/tui/screens/filepicker.go` * `tools/internal/tools/tui/screens/fmt.go` * `tools/internal/tools/tui/screens/home.go` * `tools/internal/tools/tui/screens/init.go` * `tools/internal/tools/tui/screens/inspect.go` * `tools/internal/tools/tui/screens/settings.go` * `tools/internal/tools/tui/screens/validate.go` * `tools/internal/tools/tui/state/config.go` * `tools/internal/tools/tui/style/theme.go` * `tools/tests/golden_test.go` * `tools/tests/registry_test.go` * `tools/tests/testdata/golden/dump-out.json` * `tools/tests/testdata/golden/fmt-out.yml` * `tools/tests/testdata/golden/init-config.yml` * `tools/tests/testdata/golden/init.env` * `tools/tests/tui_logic_test.go` * `tools/tests/yamltemplate_test.go` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+44
to
+47
| - name: Install golangci-lint | ||
| run: | | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.64.5 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the ci.yml workflow to understand the context
cat -n .github/workflows/ci.yml | head -60Repository: Keksclan/goConfy
Length of output: 2079
🏁 Script executed:
#!/bin/bash
# Search for Go version setup in the workflow
rg "go-version|setup-go" .github/workflows/ci.yml -A 2 -B 2Repository: Keksclan/goConfy
Length of output: 184
🌐 Web query:
golangci-lint v1.64.5 go version compatibility requirements
💡 Result:
For golangci-lint v1.64.5:
- Officially supported Go versions: the same as the Go team policy (the 2 latest Go minor versions). For the v1.64.x line (released Feb 2025), that corresponds to Go 1.24 and Go 1.23. [1]
- Upper bound rule: golangci-lint supports Go versions ≤ the Go version it was compiled with. [1]
- v1.64.x specifically added Go 1.24 support (starting v1.64.2), and the project notes building its Docker images with Go 1.24, so the upstream v1.64.5 binaries/images are intended to cover up through Go 1.24. [2]
Sources:
[1] golangci-lint FAQ (“Which Go versions are supported?”) https://golangci-lint.run/docs/welcome/faq/
[2] golangci-lint v1 changelog (v1.64.2/v1.64.3 notes incl. “go1.24 support” / “Build Docker images with go1.24”) https://golangci.github.io/legacy-v1-doc/product/changelog/
Fix golangci-lint installation to match Go 1.26.
The current installation uses v1.64.5, which was built with Go 1.24. golangci-lint supports Go versions only up to the version it was compiled with, so it cannot run on Go 1.26. Additionally, pulling the installer from the master branch is mutable and unreliable.
Use go install to compile golangci-lint locally with the target Go version:
Proposed fix
- name: Install golangci-lint
run: |
- curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.64.5
+ echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH"
+ go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.64.5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 44 - 47, The CI step that installs
golangci-lint currently pipes the mutable master install script and downloads
v1.64.5 (built with Go 1.24), which is incompatible with Go 1.26; replace that
step to build/install golangci-lint with the current Go toolchain using go
install github.com/golangci/golangci-lint/cmd/golangci-lint@<desired-version>
(or `@latest`) instead of the curl installer, and ensure the runner's PATH
includes $(go env GOPATH)/bin or set GOBIN so the installed golangci-lint binary
is available to subsequent steps.
| - **MultiError Support**: Validation errors are now aggregated into a `MultiError` for better feedback. | ||
| - **Enhanced Error Reporting**: `FieldError` now includes `Layer`, `Path`, `Line`, and `Column` information where applicable. | ||
| - **Deterministic Generator**: Golden tests in `tools/tests` ensure deterministic output for `goconfygen`. | ||
| - **Release Checklist**: Added `docs/RELEASE_CHECKLIST.md` for standardized releases. |
There was a problem hiding this comment.
Release checklist path appears inconsistent with current docs naming.
This entry references docs/RELEASE_CHECKLIST.md, while the PR documentation references RELEASE.md. Please align to the actual file path to avoid stale links.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 13, The CHANGELOG.md entry references
docs/RELEASE_CHECKLIST.md but the actual document is named RELEASE.md; update
the release checklist line in CHANGELOG.md to reference the correct path/name
(e.g., RELEASE.md or docs/RELEASE.md to match the repo), ensuring the text and
link point to the repository’s actual release document name used elsewhere in
the PR.
Comment on lines
+32
to
+34
| if e.Line > 0 { | ||
| sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column)) | ||
| } |
There was a problem hiding this comment.
Consider gating column output on Column > 0.
When Line > 0 but Column is unset (zero), the error message will include "col 0", which may confuse users into thinking the error is at column 0 rather than the column being unknown.
🛠️ Proposed fix
if e.Line > 0 {
- sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column))
+ if e.Column > 0 {
+ sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column))
+ } else {
+ sb.WriteString(fmt.Sprintf("line %d: ", e.Line))
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if e.Line > 0 { | |
| sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column)) | |
| } | |
| if e.Line > 0 { | |
| if e.Column > 0 { | |
| sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column)) | |
| } else { | |
| sb.WriteString(fmt.Sprintf("line %d: ", e.Line)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@errors.go` around lines 32 - 34, The current error message always prints "col
0" when Line > 0 but Column is 0; update the conditional around the
sb.WriteString(fmt.Sprintf("line %d, col %d: ", e.Line, e.Column)) so that
column is only included when e.Column > 0 (e.g., print "line N: " if Column is
zero and "line N, col M: " when Column > 0), locating the change near the code
referencing e.Line and e.Column and the sb.WriteString call.
| GRPC_PORT=60051 | ||
| GRPC_TIMEOUT="5s" | ||
| APP_VERSION_DISPLAY=false | ||
| APP_VERSION_DISPLAY=true |
There was a problem hiding this comment.
Resolve dotenv key ordering to satisfy linter
Line 4 introduces an ordering warning (UnorderedKey). If dotenv-linter is CI-gated, this can fail validation; move APP_VERSION_DISPLAY above GRPC_PORT.
Suggested change
APP_NAME="todo-service-dev"
+APP_VERSION_DISPLAY=true
GRPC_PORT=60051
GRPC_TIMEOUT="5s"
-APP_VERSION_DISPLAY=true🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 4-4: [UnorderedKey] The APP_VERSION_DISPLAY key should go before the GRPC_PORT key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/dotenv/.env` at line 4, Move the APP_VERSION_DISPLAY entry so dotenv
keys are in the expected order: place the APP_VERSION_DISPLAY key above the
GRPC_PORT key in the .env file to resolve the UnorderedKey linter warning
(reorder the APP_VERSION_DISPLAY and GRPC_PORT entries so APP_VERSION_DISPLAY
appears first).
Comment on lines
+20
to
+22
| if wrapped, ok := wrapError(err); ok { | ||
| return wrapped | ||
| } |
There was a problem hiding this comment.
Preserve the original decode error when returning fieldError.
Line 21 currently returns only the parsed wrapper, which drops the original strict decode error context. Keep both errors in the chain so callers still get structured field info and full decode diagnostics.
💡 Proposed fix
import (
"bytes"
+ "errors"
"fmt"
"regexp"
"strconv"
@@
if err := dec.Decode(target); err != nil {
if wrapped, ok := wrapError(err); ok {
- return wrapped
+ return fmt.Errorf("strict YAML decode: %w", errors.Join(wrapped, err))
}
return fmt.Errorf("strict YAML decode: %w", err)
}Also applies to: 28-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/decode/strict.go` around lines 20 - 22, The current return of just
the parsed wrapper from wrapError(err) discards the original decode error;
update the branches that currently do "if wrapped, ok := wrapError(err) { return
wrapped }" (including the other occurrences in the 28-40 region) to return a
combined error that preserves both the field-structured wrapper and the original
err (e.g., by joining or wrapping them so both are in the chain). Locate
wrapError and the code paths that produce fieldError and ensure the returned
error includes both the wrapped value and the original err (use errors.Join or
an appropriate wrapping pattern) so callers receive the field metadata plus full
decode diagnostics.
Comment on lines
+117
to
+120
| t.Run("MissingRequiredEnvMacro", func(t *testing.T) { | ||
| // Clear env | ||
| os.Unsetenv("DB_PASSWORD") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the integration test file to see the actual environment mutation code
cd tests/integration
wc -l integration_test.goRepository: Keksclan/goConfy
Length of output: 83
🏁 Script executed:
# Read lines around 117-120 and 168-172 to see the actual code
sed -n '115,125p' tests/integration/integration_test.go
echo "---"
sed -n '166,176p' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 476
🏁 Script executed:
# Search for all os.Unsetenv, os.Setenv, and t.Setenv patterns in the test file
rg -n --type=go 'os\.(Unsetenv|Setenv)\(|t\.Setenv\(' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 229
🏁 Script executed:
# Read more context around the unsetenv calls and their surrounding test structure
sed -n '115,135p' tests/integration/integration_test.go
echo "---"
sed -n '166,185p' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 968
🏁 Script executed:
# Also check if there's any cleanup or deferred restoration after these unsetenv calls
sed -n '115,160p' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 1127
🏁 Script executed:
# Get the full TestNegativeScenarios function to see all subtests and any cleanup
sed -n '114,200p' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 2041
🏁 Script executed:
# Check if there's any setup/cleanup at the package level or test level
rg -n --type=go 'func init|defer|cleanup|Cleanup' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 102
🏁 Script executed:
# Check what's before and after TestNegativeScenarios to see if other tests might be affected
sed -n '1,50p' tests/integration/integration_test.go
echo "---"
sed -n '200,270p' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 2805
🏁 Script executed:
# Check if DB_PASSWORD or ONLY_IN_DOTENV are referenced elsewhere in tests
rg -n 'DB_PASSWORD|ONLY_IN_DOTENV' tests/integration/integration_test.goRepository: Keksclan/goConfy
Length of output: 434
🏁 Script executed:
# Check Go version to confirm t.Setenv is available (available since Go 1.17)
cat go.mod | head -5Repository: Keksclan/goConfy
Length of output: 136
Replace process env mutations with t.Setenv for proper test isolation
Lines 119 and 171 use os.Unsetenv() without restoring the original value. This causes environment pollution across subtests and subsequent tests, potentially making the test suite order-dependent and flaky. The codebase already uses t.Setenv() elsewhere in the same file (lines 52, 203, 239), which is the idiomatic Go approach.
🔧 Proposed fix
t.Run("MissingRequiredEnvMacro", func(t *testing.T) {
- // Clear env
- os.Unsetenv("DB_PASSWORD")
+ t.Setenv("DB_PASSWORD", "") t.Run("DotenvNoMutation", func(t *testing.T) {
// Key only in .env, not in OS env
key := "ONLY_IN_DOTENV"
- os.Unsetenv(key)
+ t.Setenv(key, "")Also applies to: 168-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/integration_test.go` around lines 117 - 120, Replace direct
process-env mutations (os.Unsetenv) in the "MissingRequiredEnvMacro" subtest
with the scoped testing helper t.Setenv to avoid leaking state; specifically
remove calls to os.Unsetenv("DB_PASSWORD") and use t.Setenv("DB_PASSWORD", "")
within the subtest so the original value is restored automatically, and do the
same for the other occurrence that currently unsets env in the same file; search
for the subtest name "MissingRequiredEnvMacro" and any other places where
os.Unsetenv is used (the file already uses t.Setenv elsewhere) and convert those
to t.Setenv to ensure test isolation.
Comment on lines
+259
to
+268
| for _, entry := range capturedReport.Entries { | ||
| if entry.Path == "db.password" || entry.Path == "secret_key" { | ||
| if !entry.IsSecret { | ||
| t.Errorf("path %q should be marked as secret in report", entry.Path) | ||
| } | ||
| if entry.ValueRedacted != "[REDACTED]" { | ||
| t.Errorf("path %q value should be redacted in report, got %q", entry.Path, entry.ValueRedacted) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert required secret paths exist in explain report
Current assertions only run if matching entries are present. If db.password or secret_key are missing entirely, this test still passes.
✅ Proposed fix
- // Verify report entries
+ // Verify report entries
+ sawDBPassword := false
+ sawSecretKey := false
for _, entry := range capturedReport.Entries {
- if entry.Path == "db.password" || entry.Path == "secret_key" {
+ if entry.Path == "db.password" || entry.Path == "secret_key" {
+ if entry.Path == "db.password" {
+ sawDBPassword = true
+ }
+ if entry.Path == "secret_key" {
+ sawSecretKey = true
+ }
if !entry.IsSecret {
t.Errorf("path %q should be marked as secret in report", entry.Path)
}
if entry.ValueRedacted != "[REDACTED]" {
t.Errorf("path %q value should be redacted in report, got %q", entry.Path, entry.ValueRedacted)
}
}
}
+ if !sawDBPassword {
+ t.Error("expected explain report entry for db.password")
+ }
+ if !sawSecretKey {
+ t.Error("expected explain report entry for secret_key")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, entry := range capturedReport.Entries { | |
| if entry.Path == "db.password" || entry.Path == "secret_key" { | |
| if !entry.IsSecret { | |
| t.Errorf("path %q should be marked as secret in report", entry.Path) | |
| } | |
| if entry.ValueRedacted != "[REDACTED]" { | |
| t.Errorf("path %q value should be redacted in report, got %q", entry.Path, entry.ValueRedacted) | |
| } | |
| } | |
| } | |
| sawDBPassword := false | |
| sawSecretKey := false | |
| for _, entry := range capturedReport.Entries { | |
| if entry.Path == "db.password" || entry.Path == "secret_key" { | |
| if entry.Path == "db.password" { | |
| sawDBPassword = true | |
| } | |
| if entry.Path == "secret_key" { | |
| sawSecretKey = true | |
| } | |
| if !entry.IsSecret { | |
| t.Errorf("path %q should be marked as secret in report", entry.Path) | |
| } | |
| if entry.ValueRedacted != "[REDACTED]" { | |
| t.Errorf("path %q value should be redacted in report, got %q", entry.Path, entry.ValueRedacted) | |
| } | |
| } | |
| } | |
| if !sawDBPassword { | |
| t.Error("expected explain report entry for db.password") | |
| } | |
| if !sawSecretKey { | |
| t.Error("expected explain report entry for secret_key") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/integration_test.go` around lines 259 - 268, The test
currently only validates properties for entries if they exist; update the logic
around capturedReport.Entries to explicitly assert that entries with Path
"db.password" and "secret_key" are present: track booleans (e.g.,
foundDBPassword, foundSecretKey) while iterating over capturedReport.Entries and
perform the IsSecret and ValueRedacted checks as already written, then after the
loop add t.Errorf/t.Fatalf assertions if foundDBPassword or foundSecretKey are
false so the test fails when those paths are missing.
Comment on lines
+64
to
87
| func TestErrorLayer(t *testing.T) { | ||
| input := []byte(` | ||
| name: myapp | ||
| port: 8080 | ||
| unknown_field: ok | ||
| server: | ||
| host: "{ENV:MISSING_VAR}" | ||
| `) | ||
| cfg, err := goconfy.Load[StrictConfig]( | ||
| _, err := goconfy.Load[StrictConfig]( | ||
| goconfy.WithBytes(input), | ||
| goconfy.WithStrictYAML(false), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error in relaxed mode: %v", err) | ||
| if err == nil { | ||
| t.Fatal("expected error for missing env var") | ||
| } | ||
|
|
||
| var fe *goconfy.FieldError | ||
| if !errors.As(err, &fe) { | ||
| t.Fatalf("expected FieldError, got %T: %v", err, err) | ||
| } | ||
|
|
||
| if fe.Layer != "base" { | ||
| t.Errorf("expected layer 'base', got %q", fe.Layer) | ||
| } | ||
| if cfg.Name != "myapp" { | ||
| t.Errorf("expected name=myapp, got %q", cfg.Name) | ||
| if fe.Path != "server.host" { | ||
| t.Errorf("expected path 'server.host', got %q", fe.Path) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestErrorLayer is environment-dependent and can be flaky.
If MISSING_VAR is present in the runner environment, this test can fail for a different reason (strict decode/unknown fields) instead of missing macro value. Make the env precondition explicit in-test.
✅ Deterministic test setup
import (
"errors"
+ "os"
"strings"
"testing"
@@
func TestErrorLayer(t *testing.T) {
+ prev, had := os.LookupEnv("MISSING_VAR")
+ _ = os.Unsetenv("MISSING_VAR")
+ t.Cleanup(func() {
+ if had {
+ _ = os.Setenv("MISSING_VAR", prev)
+ } else {
+ _ = os.Unsetenv("MISSING_VAR")
+ }
+ })
+
input := []byte(`
server:
host: "{ENV:MISSING_VAR}"
`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/strict_test.go` around lines 64 - 87, TestErrorLayer is flaky because
it relies on MISSING_VAR not being set in the environment; ensure the test
explicitly controls that precondition by saving the current value of
"MISSING_VAR", unsetting it (or setting it to empty) before calling
goconfy.Load[StrictConfig], and deferring restoration of the original value
afterward; this change should be made inside TestErrorLayer around the call to
goconfy.Load so the FieldError check (fe.Layer/fe.Path) is deterministic
regardless of runner environment.
| github.com/charmbracelet/bubbles v1.0.0 | ||
| github.com/charmbracelet/bubbletea v1.3.10 | ||
| github.com/charmbracelet/lipgloss v1.1.0 | ||
| github.com/keksclan/goConfy v0.0.0-20260219103719-a3b470266ef3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tools/go.mod =="
sed -n '1,140p' tools/go.mod
echo
echo "== go.work (if present) =="
if fd '^go\.work$' . >/dev/null; then
cat go.work
else
echo "go.work not found"
fi
echo
echo "== Local replace/use directives =="
rg -n '^\s*replace\s+github\.com/keksclan/goConfy|^\s*use\s+\.\./?|^\s*use\s+\./tools' go.work tools/go.mod || trueRepository: Keksclan/goConfy
Length of output: 1587
🏁 Script executed:
#!/bin/bash
echo "== Root go.mod (if present) =="
if [ -f go.mod ]; then
head -20 go.mod
else
echo "Root go.mod not found"
fi
echo
echo "== Directory structure around tools =="
ls -la | grep -E '^d|go\.'
echo
echo "== Check if tools dir is standalone or part of workspace =="
git ls-files '*/go.mod' | head -5Repository: Keksclan/goConfy
Length of output: 1069
Add replace directive to wire tools module to local source.
The tools submodule requires github.com/keksclan/goConfy as a pseudo-version. Without a go.work file or replace directive, cd tools && go test ./... will resolve against the module cache instead of local source changes. This creates inconsistency in local testing and CI.
Add the following to tools/go.mod:
Fix
require (
github.com/charmbracelet/bubbles v1.0.0
github.com/charmbracelet/bubbletea v1.3.10
github.com/charmbracelet/lipgloss v1.1.0
github.com/keksclan/goConfy v0.0.0-20260219103719-a3b470266ef3
gopkg.in/yaml.v3 v3.0.1
)
+
+replace github.com/keksclan/goConfy => ..📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| github.com/keksclan/goConfy v0.0.0-20260219103719-a3b470266ef3 | |
| require ( | |
| github.com/charmbracelet/bubbles v1.0.0 | |
| github.com/charmbracelet/bubbletea v1.3.10 | |
| github.com/charmbracelet/lipgloss v1.1.0 | |
| github.com/keksclan/goConfy v0.0.0-20260219103719-a3b470266ef3 | |
| gopkg.in/yaml.v3 v3.0.1 | |
| ) | |
| replace github.com/keksclan/goConfy => .. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/go.mod` at line 9, tools/go.mod depends on the pseudo-versioned module
github.com/keksclan/goConfy which causes local tests to use the module cache;
add a replace directive in tools/go.mod to point github.com/keksclan/goConfy to
your local checkout (a relative path to the local goConfy module) so local
changes are used during cd tools && go test ./... and then run go mod tidy;
ensure the replace targets the exact module name "github.com/keksclan/goConfy"
in the tools/go.mod.
Comment on lines
+77
to
+79
| oldStdout := os.Stdout | ||
| r, w, _ := os.Pipe() | ||
| os.Stdout = w |
There was a problem hiding this comment.
Handle os.Pipe() error to prevent potential nil pointer dereference.
The error from os.Pipe() is silently ignored. If pipe creation fails, r and w will be nil, causing a panic when os.Stdout = w is executed or when w.Close() is called.
🛠️ Proposed fix
- r, w, _ := os.Pipe()
+ r, w, err := os.Pipe()
+ if err != nil {
+ t.Fatalf("failed to create pipe: %v", err)
+ }
os.Stdout = w📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| oldStdout := os.Stdout | |
| r, w, _ := os.Pipe() | |
| os.Stdout = w | |
| oldStdout := os.Stdout | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatalf("failed to create pipe: %v", err) | |
| } | |
| os.Stdout = w |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/tests/golden_test.go` around lines 77 - 79, The call to os.Pipe() in
the test ignores its error and can yield nil r/w; update the golden_test.go
block around oldStdout, r, w to capture the error (r, w, err := os.Pipe()),
check err and fail the test (e.g., t.Fatalf) if non-nil, and ensure os.Stdout is
only set to w after a successful pipe creation; also keep the existing restore
logic for oldStdout and close r/w in defers to avoid nil dereference.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to repository hygiene, CI/CD processes, documentation, and the separation of core and tooling modules. The most important changes include adding a comprehensive CI workflow, enforcing a clean repository policy, updating documentation to clarify the split between core and tools, and providing a new example configuration file. These changes ensure a more maintainable, secure, and user-friendly project.
Repository Hygiene & CI/CD:
.github/workflows/ci.yml) to run verification steps (formatting, linting, tests, race detector, vulnerability checks) and to guard against committed binaries or executables.Makefilewith unified targets (verify,test,lint,fmt-check,vulncheck, and more) to standardize local and CI checks.CONTRIBUTING.mdwith new repository hygiene rules, emphasizing a source-only policy and providing clear build instructions for core and tools modules.Documentation & Release Process:
README.md,CHANGELOG.md, anddocs/CLI.mdto clarify the separation between the core runtime and optional tooling (goconfygen,goconfytui), update installation/build instructions, and document new features and changes. [1] [2] [3] [4] [5] [6] [7] [8] [9]RELEASE.mdto document the release process, including versioning, tagging, and publishing guidelines.Configuration & Examples:
config.example.ymlfile demonstrating best practices and configuration conventions for users.Changelog & Feature Tracking:
CHANGELOG.mdwith new features (integration tests, error aggregation, deterministic generator), repository hardening measures, improved error reporting, and fixed macro redaction.These changes collectively improve project maintainability, developer experience, and security.
Summary by CodeRabbit
New Features
Bug Fixes
Chores