refactor(NATS-6): remove template system dead code for v2.0 - #550
Merged
Conversation
Complete the template-to-programmatic migration by removing all remaining template-era artifacts: - Delete internal/converter/compat.go (backward-compat shim, zero callers) - Remove Config.Template, Config.Engine, Config.UseTemplate fields - Remove ExportConfig.Template field and all accessor methods - Remove ValidEngines variable and validateEngine() from validation - Remove template/engine entries from config show, init, and validate - Clean template references from example-config.yaml, README, CONTRIBUTING - Update ~92 test call sites to use builder.NewMarkdownBuilder() directly All 30 packages compile and pass tests. Net deletion of 335 lines. BREAKING CHANGE: Config file keys `template`, `engine`, `use_template`, and `export.template` are no longer recognized. Environment variables OPNDOSSIER_TEMPLATE, OPNDOSSIER_ENGINE, and OPNDOSSIER_EXPORT_TEMPLATE are no longer effective. Users should remove these from their config files. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
dosubot
Bot
added
the
size:L
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Require conventional commit format per https://www.conventionalcommits.org/en/v1.0.0/. Skipped for dependabot and dosubot.
🟢 Full CI must passWonderful, this rule succeeded.All CI checks must pass. Activates for non-bot authors, or dependabot when files exist outside .github/workflows/.
🟢 Do not merge outdated PRsWonderful, this rule succeeded.Make sure PRs are within 10 commits of the base branch before merging
|
dosubot
Bot
added
breaking_change
Contributor
Summary by CodeRabbit
WalkthroughRemoved template-related configuration, validation, and compatibility shims; deleted the converter compat wrapper; updated tests and docs to use the programmatic Markdown builder from the builder package; added an end-to-end configuration guide and removed embedded-template assertions from build tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Comment |
Contributor
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: opnDossier configuration
|
Contributor
There was a problem hiding this comment.
Pull request overview
Completes the v2.0 migration from legacy template-driven markdown generation to the programmatic builder by removing remaining dead compatibility/config plumbing and updating tests/docs to reference internal/converter/builder directly.
Changes:
- Removed the legacy converter compatibility shim (
internal/converter/compat.go) and eliminated template/engine-related configuration and validation paths. - Updated converter tests/benchmarks to instantiate the markdown builder via
internal/converter/builder. - Cleaned documentation and example configuration to remove template/engine references.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Removes the legacy “template-based reports” mention. |
| internal/converter/markdown_utils_test.go | Switches builder construction to internal/converter/builder. |
| internal/converter/markdown_transformers_test.go | Updates tests/benchmarks to use a builder instance from the builder package. |
| internal/converter/markdown_security_test.go | Updates security tests to use the builder package. |
| internal/converter/markdown_integration_test.go | Updates integration tests to use builderPkg.NewMarkdownBuilder(). |
| internal/converter/markdown_formatters_test.go | Updates performance benchmarks to use the builder package. |
| internal/converter/markdown_builder_test.go | Updates main builder tests and interface-implementation assertion to builder package types. |
| internal/converter/markdown_bench_test.go | Updates benchmark harness to use the builder package. |
| internal/converter/compat.go | Deletes the legacy compatibility aliases/wrappers. |
| internal/config/validation.go | Removes engine validation and the ValidEngines enum. |
| internal/config/validation_test.go | Removes/adjusts tests that asserted engine validation errors. |
| internal/config/config.go | Removes template/engine fields, defaults, env bindings, and accessor methods. |
| internal/config/config_test.go | Removes template/engine expectations from config load/override tests. |
| internal/config/config_coverage_test.go | Removes engine/template-related indirect validation and getter coverage tests. |
| example-config.yaml | Removes template/engine/use_template configuration documentation/examples. |
| docs/development/xml-structure-research.md | Markdown table formatting cleanup in research documentation. |
| CONTRIBUTING.md | Updates examples to use the programmatic builder directly. |
| cmd/root.go | Removes Engine from the lightweight default config. |
| cmd/root_coverage_test.go | Removes assertion for the removed Engine default. |
| cmd/config_validate.go | Removes template/engine keys from the known-key allowlist. |
| cmd/config_show.go | Stops displaying template/engine config values. |
| cmd/config_init.go | Removes template/engine entries from generated config template. |
| cmd/audit_integration_test.go | Removes engine constant/field usage in audit integration tests. |
Comment on lines
286
to
316
| // findUnknownKeys checks for unknown configuration keys. | ||
| func findUnknownKeys(raw map[string]any) []string { | ||
| knownKeys := map[string]bool{ | ||
| "input_file": true, | ||
| "output_file": true, | ||
| "verbose": true, | ||
| "quiet": true, | ||
| "theme": true, | ||
| "format": true, | ||
| "template": true, | ||
| "sections": true, | ||
| "wrap": true, | ||
| "engine": true, | ||
| "use_template": true, | ||
| "json_output": true, | ||
| "minimal": true, | ||
| "no_progress": true, | ||
| "display": true, | ||
| "export": true, | ||
| "logging": true, | ||
| "validation": true, | ||
| } | ||
|
|
||
| knownNestedKeys := map[string]map[string]bool{ | ||
| "display": { | ||
| "width": true, | ||
| "pager": true, | ||
| "syntax_highlighting": true, | ||
| }, | ||
| "export": { | ||
| "format": true, | ||
| "directory": true, | ||
| "template": true, | ||
| "backup": true, | ||
| }, |
There was a problem hiding this comment.
config validate will now report legacy keys (e.g., template, engine, use_template, export.template) as unknown because they were removed from the knownKeys/knownNestedKeys allowlist. This conflicts with the PR description that these keys are “silently ignored”; users validating an older config will see warnings. Consider either (a) keeping these keys in the allowlist as deprecated (optionally emitting a specific deprecation warning), or (b) updating the PR description/docs to reflect the warning behavior.
Comment on lines
300
to
336
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| builder := converter.NewMarkdownBuilder() | ||
| result := builder.FilterSystemTunables(tt.tunables, tt.includeTunables) | ||
| mb := builder.NewMarkdownBuilder() | ||
| result := mb.FilterSystemTunables(tt.tunables, tt.includeTunables) | ||
| assert.Len(t, result, tt.expected) | ||
| }) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ##### Performance Benchmarks | ||
|
|
||
| ```go | ||
| func BenchmarkMarkdownBuilder_CalculateSecurityScore(b *testing.B) { | ||
| builder := converter.NewMarkdownBuilder() | ||
| mb := builder.NewMarkdownBuilder() | ||
| config := loadTestConfig() | ||
|
|
||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| _ = builder.CalculateSecurityScore(config) | ||
| _ = mb.CalculateSecurityScore(config) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ##### Integration Tests | ||
|
|
||
| ```go | ||
| func TestMarkdownBuilder_BuildStandardReport(t *testing.T) { | ||
| // Load test configuration | ||
| config := loadTestConfig("testdata/sample-config.xml") | ||
|
|
||
| // Generate report | ||
| builder := converter.NewMarkdownBuilder() | ||
| report, err := builder.BuildStandardReport(config) | ||
| mb := builder.NewMarkdownBuilder() | ||
| report, err := mb.BuildStandardReport(config) | ||
|
|
There was a problem hiding this comment.
These examples use builder.NewMarkdownBuilder() but don’t show (or reference) the required import path/alias for internal/converter/builder. Since builder is also a common local identifier (e.g., strings.Builder), copying these snippets as-is is likely to confuse readers and won’t compile without additional context. Consider adding an explicit import example (e.g., builderPkg ".../internal/converter/builder") or using the alias consistently in the snippets.
dosubot
Bot
added
size:XL
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration.md`:
- Around line 1-3: The configuration guide file docs/configuration.md is in the
top-level docs folder but must live under the structured docs tree; move this
file into the user-guide subtree (e.g., docs/user-guide/configuration.md),
update any internal relative links or frontmatter if present, and adjust
navigation or sidebar references so the new path is included in the docs index;
ensure the filename remains configuration.md and that any CI/doc-build
references expecting docs/configuration.md are updated to the new
docs/user-guide path.
- Around line 108-177: The documented accepted-value enums (e.g., keys 'theme',
top-level 'format', and 'logging.level') diverge from the in-tree validation
logic and tests; first reconcile the source-of-truth by updating the validation
code/tests or vice versa so a single canonical set of allowed values exists in
code, then regenerate the docs to reference that canonical set; specifically,
update the validation logic/tests (the in-tree validation implementation and its
unit tests) to reflect the intended allowed values, and replace the repeated
tables in the docs/configuration.md with values pulled from that canonical
source so 'theme', 'format', and 'logging.level' are consistent across
implementation, tests, and docs.
- Around line 490-496: Update the troubleshooting snippet that currently
suggests relaxing config permissions to 0644: change the guidance text to
require owner-only permissions and replace the chmod invocation (the line with
"chmod 644 ~/.opnDossier.yaml") to use 0600 (e.g., "chmod 600
~/.opnDossier.yaml"); ensure the surrounding comment/description reflects that
the config must be owner-readable/writable only rather than broadly readable.
In `@internal/config/config.go`:
- Around line 41-54: Detect deprecated keys (template, engine, use_template,
export.template) before decoding the config and surface a clear warning or
validation error so they aren't silently ignored; in the config load path that
produces a Config (e.g., wherever you parse/unmarshal into the Config struct),
read the raw map or YAML/JSON into an intermediate map[string]interface{} first,
check for those keys (including nested "export" → "template"), and if any are
present log or return a validation error that names the offending keys and
suggests migration, then only proceed to decode into the Config struct if the
check passes or after the user acknowledges the warning.
In `@internal/config/validation_test.go`:
- Around line 586-595: The test should assert the specific validation failures
instead of just Count >= 2: in the "invalid config returns errors" subtest
(using Config, LoggingConfig and calling ValidateV2), replace the
assert.GreaterOrEqual(t, errs.Count(), 2) with explicit checks that the returned
error set contains the Theme validation error and the Logging.Level validation
error (e.g. assert that errs reports errors for the Theme field and for
Logging.Level) and then assert errs.Count() == 2 to lock the exact expected
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5648641-4302-4b63-8193-62e7ac8d5ae0
📒 Files selected for processing (24)
CONTRIBUTING.mdREADME.mdcmd/audit_integration_test.gocmd/config_init.gocmd/config_show.gocmd/config_validate.gocmd/root.gocmd/root_coverage_test.godocs/configuration.mddocs/development/xml-structure-research.mdexample-config.yamlinternal/config/config.gointernal/config/config_coverage_test.gointernal/config/config_test.gointernal/config/validation.gointernal/config/validation_test.gointernal/converter/compat.gointernal/converter/markdown_bench_test.gointernal/converter/markdown_builder_test.gointernal/converter/markdown_formatters_test.gointernal/converter/markdown_integration_test.gointernal/converter/markdown_security_test.gointernal/converter/markdown_transformers_test.gointernal/converter/markdown_utils_test.go
💤 Files with no reviewable changes (11)
- cmd/config_init.go
- README.md
- cmd/root.go
- cmd/audit_integration_test.go
- cmd/root_coverage_test.go
- example-config.yaml
- cmd/config_show.go
- internal/config/validation.go
- internal/config/config_test.go
- internal/converter/compat.go
- cmd/config_validate.go
…gression tests - Rename build_test.go functions from template-era names (TestBinaryWithEmbeddedTemplates → TestBinaryHelp, etc.) and remove dead assertions for strings the binary no longer produces - Add regression tests for deprecated config keys (engine, template, use_template, export.template) being flagged as unknown - Rename TestMarkdownBuilder_TemplateParityValidation to TestMarkdownBuilder_ReportContentValidation Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
breaking_change
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build_test.go`:
- Around line 120-127: The test currently only logs an error when `convert`
returns err (the `if err != nil { s.T().Logf(...) }` block), allowing failures
to pass; change this to fail the test on error by replacing the logging with a
hard assertion such as `s.Require().NoError(err, "convert execution failed;
output: %s", output)` or `s.T().Fatalf(...)`, and apply the same change at the
other occurrence (the second `if err != nil { s.T().Logf(...) }` block) so any
non-nil `err` from `convert` causes the test to fail and prints `output`.
In `@internal/converter/markdown_integration_test.go`:
- Around line 17-19: The test TestMarkdownBuilder_ReportContentValidation was
weakened to only use Contains assertions and must be restored to validate
template parity: update the test to load the expected golden baseline(s) for the
standard/comprehensive reports and assert exact equality (or normalized
equality) between the programmatic output from your MarkdownBuilder and the
golden content, keeping the existing Contains checks only as supplementary
assertions; locate the test function TestMarkdownBuilder_ReportContentValidation
and replace the loose substring assertions with a golden-file comparison (or use
an existing helper such as a compareWithGolden/normalizeAndCompare helper if
present) to ensure template parity is enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bdf66b5-b8ae-4f0f-8de9-4f63a062255e
📒 Files selected for processing (3)
build_test.gocmd/config_validate_test.gointernal/converter/markdown_integration_test.go
coderabbitai
Bot
added
documentation
- Tighten ValidateV2 test to assert specific field errors instead of count >= 2 (CodeRabbit nitpick) - Make build_test convert assertions fail on error instead of logging (CodeRabbit) - Fix chmod 644 → 600 in docs/configuration.md troubleshooting (CodeRabbit security) - Add import path comment to CONTRIBUTING.md code example (Copilot) - Update convert usage to show multi-file support and all formats (Copilot) - Fix format enum to include text/html in docs/configuration.md Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
breaking_change
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build_test.go (1)
137-227: 🧹 Nitpick | 🔵 TrivialReduce duplicated standalone test setup to avoid drift.
TestBinaryHelp_StandaloneandTestBinaryConvert_Standalonere-implement build/run/config logic already present in suite helpers. Please extract shared helpers to keep maintenance cost down and behavior consistent.Refactor sketch
+func buildBinaryForTest(t *testing.T, tempDir string) string { + t.Helper() + binaryName := testBinaryName + if runtime.GOOS == windowsOS { + binaryName += exeExtension + } + binaryPath := filepath.Join(tempDir, binaryName) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "go", "build", "-o", binaryPath, ".") + cmd.Dir = "." + output, err := cmd.CombinedOutput() + require.NoError(t, err, "Failed to build binary: %s", string(output)) + return binaryPath +}As per coding guidelines:
**/*_test.go: "Extract shared test setup and assertions intotest_helpers.gofiles and use subtests to reduce duplication across similar test files (e.g., JSON and YAML tests)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build_test.go` around lines 137 - 227, The two tests duplicate building and running the binary plus creating a temp config; extract that shared logic into helpers (e.g., create a new test_helpers.go with functions buildTestBinary(ctxTimeout time.Duration, outName string) (binaryPath string, err error), runBinaryFromDir(ctxTimeout time.Duration, binaryPath string, args ...string) (output string, err error), and writeTestConfig(dir, filename, content string) error) and replace the inline code in TestBinaryHelp_Standalone and TestBinaryConvert_Standalone to call these helpers (use buildTestBinary to produce binaryPath, writeTestConfig to create test-config.xml, and runBinaryFromDir for "--help" and "convert" invocations), ensuring the helpers set cmd.Dir appropriately and return combined output and errors for assertions.
♻️ Duplicate comments (1)
internal/config/validation_test.go (1)
591-595:⚠️ Potential issue | 🟡 MinorLock this subtest to exactly two validation failures.
These field checks still pass if an unrelated validator starts failing too. Since this fixture only makes
themeandlogging.levelinvalid, asserterrs.Count() == 2here as well.Suggested change
require.NotNil(t, errs) assert.True(t, errs.HasErrors()) assertFieldError(t, errs, "theme", true) assertFieldError(t, errs, "logging.level", true) + assert.Equal(t, 2, errs.Count())As per coding guidelines:
**/*_test.go: Test files: Follow test proportionality principles - meaningful tests only. Verify tests cover critical functionality and real edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/validation_test.go` around lines 591 - 595, Test currently only asserts specific field errors but not that there are exactly two failures; add an assertion to lock the subtest to exactly two validation failures by checking errs.Count() == 2 (use require.Equal or assert.Equal in the same test) immediately after calling cfg.ValidateV2() and before assertFieldError checks so that only "theme" and "logging.level" are allowed failures; reference the existing cfg.ValidateV2() call and errs variable when adding the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 284-285: Replace the commented placeholder import with a real
import block that includes the converter builder package so the later call to
builder.NewMarkdownBuilder() resolves; specifically remove the line "// import
\"github.com/EvilBit-Labs/opnDossier/internal/converter/builder\"" and add a
minimal import (...) declaration that imports
"github.com/EvilBit-Labs/opnDossier/internal/converter/builder" (and any stdlib
packages used in the snippet) so the example is copy/paste runnable.
In `@docs/configuration.md`:
- Around line 96-128: The docs are missing the debug configuration key in the
YAML reference and associated tables; add an entry for the "debug" boolean key
(supported in config/defaults) to the configuration YAML block (with
description: enable debug-level logging, default false), add a corresponding row
to the environment variables table (e.g., DEBUG or APP_DEBUG mapping to debug)
and add it to the basic settings table with default value and short description;
update any other referenced sections mentioned (lines ~200-223 and ~306-318) to
consistently mention "debug" so all three locations reflect the same default and
meaning.
- Around line 272-277: Update the convert flag table to use the actual CLI flag
name `--section` (singular) instead of `--sections`; locate the table entry for
the convert command and replace the `--sections` header/row with `--section`,
keeping the same description ("Sections to include (comma-separated)") and
default value ("all") so examples match the CLI behavior and copy-paste works
for the convert command.
---
Outside diff comments:
In `@build_test.go`:
- Around line 137-227: The two tests duplicate building and running the binary
plus creating a temp config; extract that shared logic into helpers (e.g.,
create a new test_helpers.go with functions buildTestBinary(ctxTimeout
time.Duration, outName string) (binaryPath string, err error),
runBinaryFromDir(ctxTimeout time.Duration, binaryPath string, args ...string)
(output string, err error), and writeTestConfig(dir, filename, content string)
error) and replace the inline code in TestBinaryHelp_Standalone and
TestBinaryConvert_Standalone to call these helpers (use buildTestBinary to
produce binaryPath, writeTestConfig to create test-config.xml, and
runBinaryFromDir for "--help" and "convert" invocations), ensuring the helpers
set cmd.Dir appropriately and return combined output and errors for assertions.
---
Duplicate comments:
In `@internal/config/validation_test.go`:
- Around line 591-595: Test currently only asserts specific field errors but not
that there are exactly two failures; add an assertion to lock the subtest to
exactly two validation failures by checking errs.Count() == 2 (use require.Equal
or assert.Equal in the same test) immediately after calling cfg.ValidateV2() and
before assertFieldError checks so that only "theme" and "logging.level" are
allowed failures; reference the existing cfg.ValidateV2() call and errs variable
when adding the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cff3856e-0d8d-4891-9ca7-798308445868
📒 Files selected for processing (4)
CONTRIBUTING.mdbuild_test.godocs/configuration.mdinternal/config/validation_test.go
Member
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
This pull request spent 5 minutes 28 seconds in the queue, including 5 minutes 13 seconds running CI. Required conditions to merge
|
39 tasks
unclesp1d3r
added a commit
that referenced
this pull request
Apr 15, 2026
- CONTRIBUTING.md: replace commented placeholder import with a real import block in the builder unit test example so the snippet is copy/paste runnable. - docs/configuration.md: document the `debug` config key in the YAML reference, environment variable table, and basic settings table. - docs/configuration.md: correct the `convert` flag table to use `--section` (singular), matching the actual CLI flag. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
16 tasks
mergify Bot
pushed a commit
that referenced
this pull request
Apr 15, 2026
## Summary Follow-up to PR #550 addressing 3 unresolved CodeRabbit review threads that landed after the PR merged. Pure documentation — no runtime behavior change. **Impact:** 3 files, +24/-11 lines · **Risk:** 🟢 Low · **Review time:** ~3 minutes ## What Changed ### 📝 Documentation - **M** `CONTRIBUTING.md` — In the builder unit-test code example, replaced a single commented placeholder import line with a real `import (...)` block (`testing`, `builder`, `common`, `testify/assert`) so the snippet is copy/paste runnable. - **M** `docs/configuration.md` — Added the `debug` config key to three places it was missing from: the YAML reference block, the env-var table (`OPNDOSSIER_DEBUG`), and the basic settings table. - **M** `docs/configuration.md` — Fixed `--sections` → `--section` in the `convert` flag table to match the actual CLI flag. - **M** `docs/user-guide/configuration.md` — Dosu-bot auto-sync mirroring the above corrections to the MkDocs user-guide copy. ## Why These Changes | Thread | Reviewer concern | Evidence this is real | |---|---|---| | [#550 r3070976086](#550 (comment)) | Copy/paste-broken import example | Placeholder was commented out; snippet called `builder.NewMarkdownBuilder()` with no resolvable import | | #550 thread `PRRT_...56bzAh` | `debug` undocumented | `internal/config/config.go:46` (field), `:98` (default), `cmd/root.go:201` (flag) — exists but invisible to users | | #550 thread `PRRT_...56bzAj` | Docs say `--sections`; CLI says `--section` | `cmd/convert.go:100` registers `section` (singular); example at `:181` uses `--section` | ## Type of Change - [x] 📝 Documentation update - [ ] 🐛 Bug fix - [ ] ✨ New feature - [ ] 💥 Breaking change ## How This Has Been Tested - [x] `pre-commit run --files CONTRIBUTING.md docs/configuration.md` — passes (mdformat clean) - [x] Verified `debug` field exists via `grep -rn '\"debug\"\|OPNDOSSIER_DEBUG' internal/config/ cmd/` - [x] Verified `--section` (singular) is the registered flag via `grep -rn 'section' cmd/convert.go cmd/display.go` - [x] No code changes — no unit/integration tests affected ## Breaking Changes None. Documentation-only. ## Review Checklist ### Documentation - [x] Documentation matches current CLI behavior - [x] Examples are copy/paste runnable - [x] No divergence between three `debug` reference sites - [x] Cross-references (YAML ref, env-var table, basic settings) stay in sync - [x] User-guide copy (`docs/user-guide/configuration.md`) stays in sync via Dosu ### Process - [x] DCO sign-off present (`-s` flag) - [x] Conventional commit format (`docs:`) - [x] Source threads on #550 resolved (already replied + resolved via GraphQL) ## Related - Parent PR: #550 (merged, template system removal) - CodeRabbit threads: 3 resolved on #550 linking back to this follow-up --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io> Co-authored-by: dosubot[bot] <131922026+dosubot[bot]@users.noreply.github.com>
3 tasks
mergify Bot
pushed a commit
that referenced
this pull request
Apr 16, 2026
## Summary Follow-up cleanup for the NATS-6 template-system removal (#550, #552). 5 residuals were surfaced by an audit against the original ticket's acceptance criteria and fixed together. ### What changed - **`addSharedTemplateFlags` renamed to `addSharedContentFlags`** across 7 files. The function registers only content and formatting flags (`--section`, `--wrap`, `--no-wrap`, `--include-tunables`, `--comprehensive`). The old name and its "retained for backward compatibility" doc comment were both fiction. - **Deleted `TestAddSharedContentFlagsComprehensive`**. Its assertions were a strict subset of `TestAddSharedContentFlagsRegistersFlags`. The `expectPanic` / `defer recover()` scaffolding was dead code since both test cases had `expectPanic: false`. - **Dropped 6 redundant `defer os.RemoveAll(tmpDir)` blocks** in `internal/export/file_test.go`. Every `tmpDir` came from `t.TempDir()`, which already registers cleanup via `t.Cleanup()`. 3 other tests in the same file already omit the defer. - **Removed the dead `"template"` substring branch** in a `convert_test.go` error-class assertion. Post-NATS-6 the convert command cannot emit a `"template"` error; only `parse` and `generator` are reachable. - **Cleaned up stale comments** in `file_test.go` that claimed "Uses templates from internal/templates" (directory removed in NATS-6) and in `shared_flags_test.go` that read awkwardly after the enclosing function was renamed. Net: +23 / −120 across 8 files. No behavior change. CodeRabbit: 0 findings. ## Test Plan - [x] `just ci-check` passes locally - [x] `grep addSharedTemplateFlags` returns zero hits across the repo - [x] `TestAddSharedContentFlagsRegistersFlags` still asserts the same 5 positive and 5 negative flags --- [](https://github.com/EveryInc/compound-engineering-plugin) -D97757?logo=claude&logoColor=white) --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
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.
Summary
internal/converter/compat.go— backward-compat shim with zero external callersTemplate,Engine,UseTemplatefields fromConfigandExportConfigstructs, along with all accessor methods, Viper defaults, env bindings, and engine validationbuilder.NewMarkdownBuilder()directlyexample-config.yaml,README.md,CONTRIBUTING.md,config_init.go,config_show.go,config_validate.goNet result: 23 files changed, -335 lines. All 30 packages compile and pass tests.
Context
The template-to-programmatic migration was ~90% complete. All
.tmplfiles were already deleted, all template CLI flags removed, and the full programmatic builder pattern implemented (7,289 lines ininternal/converter/builder/). This PR completes the final 10% — removing dead config fields, unused compat shims, stale validation, and documentation references.Breaking Changes
template,engine,use_template, andexport.templateare no longer recognized (viper silently ignores them — no crash, but no effect)OPNDOSSIER_TEMPLATE,OPNDOSSIER_ENGINE,OPNDOSSIER_EXPORT_TEMPLATEare no longer effectiveconverter.ReportBuilder,converter.MarkdownBuilder,converter.NewMarkdownBuilder(),converter.NewMarkdownBuilderWithConfig()type aliases/functions removed (internal packages — no external consumers)Jira: NATS-6
Test plan
go build ./...— clean compilego test ./... -count=1— all 30 packages passjust ci-check— passes (excluding pre-existing flaky perf benchmark)build_test.gonaming — tracked as todo Feature: Support alternative output formats for convert command (text, HTML) #59)Post-Deploy Monitoring & Validation
No additional operational monitoring required: this is a dead code removal with no runtime behavior change. The binary output is identical before and after this refactor.