fix(parser): liberal boolean parsing for OPNsense config.xml (#558) - #577
Merged
Conversation
OPNsense 26.1 emits "on" into schema fields previously typed as Go `int` (DNSAllowOverride, UseVirtualTerminal, Disable*Offloading, etc.), causing `strconv.ParseInt: parsing "on"` to abort the entire parse with no element context. This change introduces a layered boolean-parsing stack in the schema layer and migrates the affected OPNsense toggle fields to `BoolFlag`: - New `pkg/schema/shared/` package: `IsValueTrue` / `IsValueFalse` as the single canonical truthy/falsy parser (1|on|yes|true|enable|enabled and their falsy counterparts, case-insensitive); `FlexBool` as a value-level liberal bool type; `FlexInt` as a liberal int sibling for future hot-fixes. - Upgraded `opnsense.BoolFlag.UnmarshalXML` to layer presence semantics over `FlexBool` parsing: absent → false, `<tag/>` → true, `<tag>body</tag>` → `IsValueTrue(body)`. Fixes a latent bug where `<tag>0</tag>` previously parsed as true. Marshal side untouched to preserve GOTCHAS §15.1 pointer-receiver invariants. - Migrated ten `opnsense.System` fields from `int` to `BoolFlag` so "on"/"yes"/"1" all parse correctly; consumers switch from `== 1` / `!= 0` to plain bool reads. - Retired `pfsense.isPfSenseValueTrue`; pfSense converter sites now consume `shared.IsValueTrue` directly, unifying the truthy vocabulary. - New `parser.WrapDecodeError` annotates XML decode errors with the element path (`field "/opnsense/system": strconv.ParseInt: ...`) across both OPNsense and pfSense parsers so future field-specific parse failures surface the offending element by name. - GOTCHAS §15 expanded with a decision rubric covering BoolFlag, FlexBool, FlexInt, and strict int/bool; addressability warning extended to the new types. Wrapper types stay in the schema layer only — `CommonDevice` fields remain native `bool`/`int`. Plan: docs/plans/2026-04-18-002-fix-issue-558-parser-on-value.md Closes #558 Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Closed
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
bug
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralized liberal boolean/integer parsing added (shared.IsValueTrue/IsValueFalse, FlexBool, FlexInt), BoolFlag.UnmarshalXML now treats empty bodies as presence=true and non-empty bodies via shared parsing, multiple schema fields migrated from int→BoolFlag, XML decode errors gain element-path context, and tests/docs updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant XML as XML Input
participant CFG as cfgparser
participant Unmarshal as Schema Unmarshaler<br/>(BoolFlag/FlexBool)
participant Shared as shared.IsValueTrue
participant Parser as Device Parser/Converter
XML->>CFG: feed bytes to parser
CFG->>Unmarshal: dec.DecodeElement(target, &se)
Unmarshal->>Unmarshal: inspect element body
alt empty body / self-closing
Unmarshal-->>CFG: set presence=true (BoolFlag)
else non-empty body
Unmarshal->>Shared: IsValueTrue(body)
Shared-->>Unmarshal: truthy/falsy result
Unmarshal-->>CFG: set value accordingly
end
CFG->>Parser: return populated schema struct
Parser->>Parser: convert schema fields (bool(...) / .Bool())
Parser-->>Common: populate common.* types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
coderabbitai
Bot
added
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #558 by making schema parsing tolerant of liberal boolean encodings (e.g., on/yes/true/enabled) in OPNsense/pfSense XML, and improves decode errors with element-path context.
Changes:
- Introduces
pkg/schema/sharedhelpers (IsValueTrue/False) plus liberal wrapper types (FlexBool,FlexInt) with XML/JSON/YAML support. - Updates OPNsense
BoolFlagand migrates severalSystemtoggle fields frominttoBoolFlag, updating converters/tests accordingly. - Adds
parser.WrapDecodeErrorand uses it in both parsers to annotate XML decode failures with an element path; adds regression/coverage tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/schema/shared/bool.go | Adds canonical shared truthy/falsy helpers. |
| pkg/schema/shared/bool_test.go | Tests shared truthy/falsy vocabulary. |
| pkg/schema/shared/flex_bool.go | Adds liberal boolean wrapper with XML/JSON/YAML support. |
| pkg/schema/shared/flex_bool_test.go | Tests FlexBool XML/JSON/YAML behavior. |
| pkg/schema/shared/flex_int.go | Adds liberal integer wrapper (numeric + truthy/falsy coercion). |
| pkg/schema/shared/flex_int_test.go | Tests FlexInt XML marshal/unmarshal and error cases. |
| pkg/schema/opnsense/common.go | Updates BoolFlag.UnmarshalXML to honor value bodies via shared truthy parser. |
| pkg/schema/opnsense/common_test.go | Expands BoolFlag test coverage for liberal vocabulary and presence semantics. |
| pkg/schema/opnsense/system.go | Migrates multiple system toggle fields from int to BoolFlag. |
| pkg/schema/opnsense/testdata/system_liberal_bool.xml | Adds OPNsense 26.1-style fixture with mixed boolean encodings. |
| pkg/schema/opnsense/system_liberal_bool_test.go | Adds #558 regression test ensuring on parses successfully. |
| pkg/schema/opnsense/opnsense.go | Updates NAT summary logic to use BoolFlag-based fields. |
| pkg/schema/opnsense/opnsense_test.go | Updates tests to set/read migrated BoolFlag fields. |
| pkg/parser/xmlutil.go | Adds WrapDecodeError helper for consistent decode error annotation. |
| internal/cfgparser/xml.go | Wraps decode errors with /opnsense/<section> element-path context. |
| internal/cfgparser/element_path_test.go | Verifies decode errors include an element path for malformed numeric fields. |
| pkg/parser/pfsense/parser.go | Wraps pfSense full-document decode errors with /pfsense path context. |
| pkg/parser/pfsense/parser_test.go | Updates expectations for new wrapped error messages. |
| pkg/parser/pfsense/converter_network.go | Switches pfSense value-boolean parsing to shared.IsValueTrue. |
| pkg/parser/pfsense/constants.go | Removes pfSense-specific truthy helper and updates guidance/comments. |
| pkg/parser/opnsense/converter.go | Updates OPNsense converter to use BoolFlag-based system fields. |
| pkg/parser/opnsense/converter_test.go | Updates converter tests for BoolFlag-based inputs. |
| docs/development/standards.md | Documents new shared boolean parsing guidance for pfSense/OPNsense. |
| GOTCHAS.md | Documents selection/semantics of BoolFlag vs FlexBool vs FlexInt and addressability caveats. |
| AGENTS.md | Updates markdown-validation guidance and other contributor rule text. |
Contributor
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: opnDossier xml-structure-research
|
dosubot
Bot
added
size:XL
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/schema/opnsense/opnsense_test.go (1)
295-304:⚠️ Potential issue | 🟡 MinorUpdate stale assertion wording after bool migration.
Line 303 still says “set to 0”, but the test now sets a bool (
false). This can confuse failure triage.✏️ Suggested tweak
- t.Error("NATSummary.PfShareForward should be false when set to 0") + t.Error("NATSummary.PfShareForward should be false when set to false")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/schema/opnsense/opnsense_test.go` around lines 295 - 304, The test TestOpnSenseDocument_NATSummary_PfShareForwardZero has a stale failure message referencing "set to 0" even though the test now sets doc.System.PfShareForward = false; update the assertion message in the test (around TestOpnSenseDocument_NATSummary_PfShareForwardZero where NewOpnSenseDocument, doc.System.PfShareForward and summary.PfShareForward are used) to say "NATSummary.PfShareForward should be false when set to false" (or similar clear wording) so failure output matches the bool migration.
🤖 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/development/standards.md`:
- Line 721: Update the wording on line 721 to stop recommending BoolFlag for
generic value-based booleans and instead recommend FlexBool (or shared helpers)
for fields that are always emitted; reserve BoolFlag only for absence-sensitive
fields. Specifically, change the guidance that mentions using `BoolFlag` in
`pkg/schema/**/*.go` to instruct developers to use `FlexBool` for value-based
booleans (or call `shared.IsValueTrue()` at conversion boundaries) and to use
`BoolFlag` only when element absence carries meaning (i.e., presence semantics).
In `@pkg/parser/opnsense/converter.go`:
- Around line 116-128: The mapping currently checks sys.DisableNATReflection
(and the other hard-coded `"yes"` checks around the other mapping at the second
occurrence) with strings.EqualFold/xmlBoolYes which misses other truthy values;
replace those call sites so DisableNATReflection uses
shared.IsValueTrue(sys.DisableNATReflection) (or shared.IsValueFalse where
appropriate) and update the same pattern for the other affected fields (the
second block around lines referencing sys.DisableNATReflection/xmlBoolYes) to
consistently use shared.IsValueTrue()/shared.IsValueFalse() instead of
strings.EqualFold/xmlBoolYes.
In `@pkg/schema/opnsense/system_liberal_bool_test.go`:
- Line 13: The tiny helper function bytesReader is only used once (bytesReader(b
[]byte) in pkg/schema/opnsense/system_liberal_bool_test.go); remove the
bytesReader function and replace its single call with bytes.NewReader(data)
where it's invoked (e.g., in the test that currently calls bytesReader). Ensure
you import bytes is still present or add it if needed and run tests to verify no
other references to bytesReader remain.
In `@pkg/schema/shared/flex_bool_test.go`:
- Line 86: The subtests in flex_bool_test.go use anonymous t.Run names which
makes test output hard to read; update the t.Run calls inside the
TestMarshal/TestUnmarshal (and any marshal-related subtests) to use descriptive
names such as "marshal true" and "marshal false" (or "unmarshal true"/"unmarshal
false" where appropriate) so each subtest for the FlexBool marshal/unmarshal
cases is clearly identified in test output; locate the t.Run("", func(t
*testing.T) { ... }) occurrences around the FlexBool marshal tests and replace
the empty string with an appropriate descriptive label.
In `@pkg/schema/shared/flex_bool.go`:
- Around line 100-104: Add compile-time interface checks for JSON and YAML by
asserting that *FlexBool implements encoding/json.Marshaler and
encoding/json.Unmarshaler and the YAML marshaler/unmarshaler types used in the
repo (e.g., gopkg.in/yaml.v3.Marshaler and gopkg.in/yaml.v3.Unmarshaler). Update
the var block that currently contains _ xml.Marshaler/_ xml.Unmarshaler to also
include _ json.Marshaler = (*FlexBool)(nil), _ json.Unmarshaler =
(*FlexBool)(nil), and the corresponding yaml.Marshaler/yaml.Unmarshaler
assertions so the compiler will catch missing method implementations for
FlexBool.
- Around line 62-74: The UnmarshalJSON implementation in FlexBool manually
strips surrounding quotes which fails for escaped JSON string values; update
FlexBool.UnmarshalJSON to detect quoted inputs and use strconv.Unquote to obtain
the proper unescaped string before passing it to IsValueTrue, falling back to
the original string if Unquote returns an error; reference the
FlexBool.UnmarshalJSON method, the FlexBool type and IsValueTrue helper, and
import strconv if needed.
In `@pkg/schema/shared/flex_int_test.go`:
- Around line 61-74: The tests only cover XML for FlexInt
(TestFlexInt_MarshalXML) but lack JSON/YAML round-trip coverage like FlexBool;
add two tests (e.g., TestFlexInt_JSON and TestFlexInt_YAML) that create a
flexIntWrap or directly use shared.FlexInt(42), marshal to JSON and YAML and
then unmarshal back asserting equality and correct string/number encoding; use
encoding/json's Marshal/Unmarshal and gopkg.in/yaml.v3 (or the same YAML helper
used in flex_bool_test.go) to exercise shared.FlexInt's
MarshalJSON/UnmarshalJSON and MarshalYAML/UnmarshalYAML implementations.
In `@pkg/schema/shared/flex_int.go`:
- Around line 120-124: Add compile-time interface checks for JSON and YAML on
FlexInt similar to the existing XML checks: ensure FlexInt implements
encoding/json's json.Marshaler and json.Unmarshaler and the YAML package's
yaml.Marshaler and yaml.Unmarshaler by adding the appropriate var assertions
referencing (*FlexInt)(nil) (e.g., _ json.Marshaler = (*FlexInt)(nil), _
json.Unmarshaler = (*FlexInt)(nil), _ yaml.Marshaler = (*FlexInt)(nil), _
yaml.Unmarshaler = (*FlexInt)(nil)) next to the existing xml checks so the
compiler will validate those implementations.
---
Outside diff comments:
In `@pkg/schema/opnsense/opnsense_test.go`:
- Around line 295-304: The test
TestOpnSenseDocument_NATSummary_PfShareForwardZero has a stale failure message
referencing "set to 0" even though the test now sets doc.System.PfShareForward =
false; update the assertion message in the test (around
TestOpnSenseDocument_NATSummary_PfShareForwardZero where NewOpnSenseDocument,
doc.System.PfShareForward and summary.PfShareForward are used) to say
"NATSummary.PfShareForward should be false when set to false" (or similar clear
wording) so failure output matches the bool migration.
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b69a27d0-d2f9-4787-87ea-bceb7014f626
📒 Files selected for processing (25)
AGENTS.mdGOTCHAS.mddocs/development/standards.mdinternal/cfgparser/element_path_test.gointernal/cfgparser/xml.gopkg/parser/opnsense/converter.gopkg/parser/opnsense/converter_test.gopkg/parser/pfsense/constants.gopkg/parser/pfsense/converter_network.gopkg/parser/pfsense/parser.gopkg/parser/pfsense/parser_test.gopkg/parser/xmlutil.gopkg/schema/opnsense/common.gopkg/schema/opnsense/common_test.gopkg/schema/opnsense/opnsense.gopkg/schema/opnsense/opnsense_test.gopkg/schema/opnsense/system.gopkg/schema/opnsense/system_liberal_bool_test.gopkg/schema/opnsense/testdata/system_liberal_bool.xmlpkg/schema/shared/bool.gopkg/schema/shared/bool_test.gopkg/schema/shared/flex_bool.gopkg/schema/shared/flex_bool_test.gopkg/schema/shared/flex_int.gopkg/schema/shared/flex_int_test.go
Addresses convergent findings from code-reviewer, silent-failure-hunter, and pr-test-analyzer on PR #577. Critical fix: - pkg/schema/shared/flex_bool.go: FlexBool.UnmarshalJSON previously hand-stripped surrounding quotes without delegating to encoding/json, so escape sequences ("\u006fn" for "on") were compared literally and silently yielded false. Now cascades json.Unmarshal through native bool, native int, and string forms so escapes are decoded before the truthy-vocabulary check. Added TestFlexBool_UnmarshalJSON_EscapeSequences. Complete the migration: - pkg/schema/pfsense/system.go: DNSAllowOverride, DisableSegmentationOffloading, and DisableLargeReceiveOffloading were still typed as `int`, reproducing the exact #558 crash on pfSense configs that emit "on"/"yes". Migrated to opnsense.BoolFlag to match the OPNsense side. Converter and parser test updated. Doc accuracy: - pkg/schema/shared/flex_bool.go: FlexBool doc previously claimed BoolFlag delegates "through FlexBool"; actually both delegate to shared.IsValueTrue. Corrected. - pkg/schema/opnsense/common.go: vocabulary list added "enable" (was listed in IsValueTrue and the decision rubric but missing from this type's docstring). - pkg/parser/pfsense/constants.go: same vocabulary list update. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
breaking_change
Captures the #558 root cause, the investigation dead ends (escape-hatch FlexInt, BoolFlag/FlexBool collapse, hand-stripped JSON quote handling), the shared-helper solution stack, why it works, and prevention rules for the next time the "strconv.ParseInt: parsing 'on'" class of bug surfaces. Key prevention guidance: - Never hand-roll truthy parsers at converter sites; use shared.IsValueTrue/shared.IsValueFalse. - BoolFlag vs FlexBool vs FlexInt vs strict: decision rubric with explicit "BoolFlag is NOT a drop-in" criteria. - WrapDecodeError for element-path annotation on every new XML decode surface. - Test both marshal and unmarshal of new schema types, including value-marshaled parent structs per GOTCHAS §15.1. - recvcheck suppression goes on the type declaration, not the accessor method. Includes historical context from prior session research: PR #461 set the clean-fork precedent for pfSense type forks; GOTCHAS §15.1 pointer-receiver pattern originated March 2026. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Both copilot-pull-request-reviewer and coderabbitai flagged that FlexInt.UnmarshalYAML passed node.Value directly to parse(), which uses strconv.Atoi. That bypasses yaml.v3's native !!int decoding, so legitimate YAML integer forms — 0x10 (hex), 0o20 (octal), 1_000 (underscored) — errored even though they are valid integers. Mirror the branching from FlexBool.UnmarshalYAML: decode via node.Decode(&int) on !!int tags, decode via node.Decode(&bool) on !!bool tags (0/1 coercion), fall through to parse() for strings and anything else. Added regression tests covering 0x10, 0o20, 1_000, -5, and native bool true. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
documentation
Member
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status🛑 Queue command has been cancelled |
39 tasks
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/solutions/runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.md`:
- Around line 162-167: Add a "Cross-Repository Impact" subsection to the
document (near the "Why This Works" or "Prevention" section) describing
downstream changes required for opnConfigGenerator: note that opnConfigGenerator
(EvilBit-Labs/opnConfigGenerator) currently pins opnDossier v1.4.0 and
constructs BoolFlag values, requires a dependency bump to get migrated types
(BoolFlag/FlexBool/FlexInt), updates to any code that constructs those fields,
test updates for numeric XML encodings, and potential opnsense-config.xsd
changes to accommodate non-integer encodings; reference BoolFlag,
shared.IsValueTrue, WrapDecodeError and the 13 migrated fields so maintainers
can locate affected code and tests.
- Line 209: The file uses the UK spelling "miscategorisation" in the guidance
text; update that term to US spelling "miscategorization" wherever it appears
(the sentence referencing strconv.ParseInt, strconv.ParseBool, strconv.Atoi and
the OPNsense/pfSense PHP pattern $config[...] == "1") so the documentation
matches the codebase's US-English convention and keeps terminology consistent.
- Around line 46-48: The fenced code block that shows the error message opnsense
parser: strconv.ParseInt: parsing "on": invalid syntax is missing a language
identifier; update that fence to include a language label such as text (e.g.,
change the opening ``` to ```text) so the markdown shows the output correctly
and satisfies static analysis requirements.
- Around line 32-36: The related_docs list in
liberal-boolean-xml-parsing-opnsense-pfsense.md references a missing doc
"docs/plans/2026-04-18-002-fix-issue-558-parser-on-value.md"; fix by either
creating that plan file with the expected content and frontmatter matching other
docs (so the reference resolves) or remove the entry
"docs/plans/2026-04-18-002-fix-issue-558-parser-on-value.md" from the
related_docs array in the file to avoid a broken link; update the change so the
related_docs list only contains existing document paths.
In `@pkg/schema/shared/flex_int_test.go`:
- Around line 161-181: The nativeForms table-driven YAML test in
TestFlexInt_YAML loops without subtests, making failures hard to identify;
change the loop to run each case as a subtest using t.Run(tc.name, func(t
*testing.T) { ... }) and move the existing unmarshalling/assertions inside that
closure (using a new local variable for tc to avoid closure capture), mirroring
the XML/JSON tests' pattern and keeping assertions on shared.FlexInt.Int().
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a578839-5feb-4919-a5a0-5032166092c1
📒 Files selected for processing (3)
docs/solutions/runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.mdpkg/schema/shared/flex_int.gopkg/schema/shared/flex_int_test.go
Reviewer flagged "miscategorisation" on line 209 of the solution doc; scanning the rest of the PR's new files surfaced the same UK spelling in five more spots in the shared/ package: - docs/solutions/runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.md — "miscategorisation" → "miscategorization" - pkg/schema/shared/flex_bool.go — "recognised"/"Unrecognised" × 3 → "recognized"/"Unrecognized" - pkg/schema/shared/flex_int.go — "recognised" × 2 → "recognized" (including the FlexInt error message for invalid values) Scope: only files created or modified in PR #577. Pre-existing docs that use "behaviour", "categorisation", etc. (e.g., contributing.md, firewall-security-controls-reference.md) remain unchanged — those are out of scope for this PR's style normalization. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Member
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
Required conditions to enter a queue
|
Contributor
Merge Queue Status
This pull request spent 5 minutes 58 seconds in the queue, including 5 minutes 44 seconds running CI. Required conditions to merge
|
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
Fixes #558 — OPNsense 26.1 emits
oninto schema fields previously typed as Goint, causingstrconv.ParseInt: parsing "on"to abort the parse with no element context. This PR introduces a layered liberal-boolean parsing stack in the schema layer, unifies three divergent truthy parsers into one canonical helper, and adds universal element-path annotation on XML decode errors across both OPNsense and pfSense parsers.Impact: 31 files changed (+1508 / −205), ~75% of the net-new lines are tests, fixtures, and docs.
CommonDevice(public consumer surface per NATS-144) remains nativebool/int— wrapper types stay in the schema layer only.Risk: 🟢 Low. Pure parser/schema layer change.
common.CommonDevicefield types unchanged. Full test suite + 3 rounds of automated review + post-merge solution doc.Review time: ~30–40 minutes for the core changes; the rest is self-explanatory tests and docs.
What Changed
🔧 New package
pkg/schema/shared/Single source of truth for the liberal truthy vocabulary
1 | on | yes | true | enable | enabled(and their falsy counterparts), case-insensitive with whitespace trimming.bool.go—IsValueTrue(s)/IsValueFalse(s)flex_bool.go—FlexBooltype: value-level liberal bool with XML/JSON/YAML marshal/unmarshal. Use when the element is always emitted.flex_int.go—FlexInttype: liberal int sibling; strict on unknown non-numeric input.🔧
opnsense.BoolFlag.UnmarshalXMLupgradedPreviously set
trueon any element presence — so<tag>0</tag>wrongly decoded totrue(latent bug that predates #558). Now:false<tag/>or<tag></tag>(empty body) →true<tag>body</tag>→shared.IsValueTrue(body)Marshal side untouched to preserve the pointer-receiver invariant from GOTCHAS §15.1.
🔧 Field migrations
System(10 fields):int → BoolFlag:DNSAllowOverride,UseVirtualTerminal,DisableVLANHWFilter,DisableChecksumOffloading,DisableSegmentationOffloading,DisableLargeReceiveOffloading,PfShareForward,LbUseSticky,RrdBackup,NetflowBackup.System(3 fields): same type change:DNSAllowOverride,DisableSegmentationOffloading,DisableLargeReceiveOffloading.pfsense.isPfSenseValueTrue; call sites inconverter_network.go(BlockPriv,BlockBogons,FarGW) useshared.IsValueTrue.strings.EqualFold(x, xmlBoolYes)forDisableNATReflectionwithshared.IsValueTrue(x)at both sites inopnsense/converter.go.🔧 Universal element-path error annotation
New
parser.WrapDecodeErrorhelper inpkg/parser/xmlutil.go. Applied to both parsers so a decode failure likestrconv.ParseInt: parsing "banana"now surfaces asfield "/opnsense/system": strconv.ParseInt: parsing "banana". Reporters on future schema/config mismatches can share the exact offending element without having to reproduce.✅ Tests
pkg/schema/shared/bool_test.go,flex_bool_test.go,flex_int_test.go— full vocabulary + XML/JSON/YAML round-trip coverage including escape sequences and non-decimal YAML integer forms (0x10,0o20,1_000).pkg/schema/opnsense/system_liberal_bool_test.go— table-driven regression for windows error parse #558 exercising all 10 migrated OPNsense fields.internal/cfgparser/element_path_test.go— proves decode errors carry the element path.testdata/system_liberal_bool.xml— reporter-scenario input (mixedon/yes/1/0/off/nobodies).common_test.gowith the full BoolFlag vocabulary matrix (including explicit-falsy regression guard for the behavior change).cmd/root_test.go/cmd/root_coverage_test.go/cmd/validate_test.go/cmd/shared_flags_coverage_test.go(surfaced while fixing lint).📝 Docs
GOTCHAS.md§15 — new decision rubric table (BoolFlagvsFlexBoolvsFlexIntvs strict) + extended §15.1 pointer-receiver caveat to cover the new types.docs/development/standards.md§Type selection — resolved intra-doc contradiction; restrictedBoolFlagrecommendation to presence-based toggles.docs/development/xml-structure-research.md— added "BoolFlag is NOT a drop-in" caveat with two migration criteria.docs/solutions/runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.md— full post-mortem solution doc (root cause, what didn't work, solution stack, prevention rules, session-history context).Why These Changes
OPNsense and pfSense share the same liberal boolean vocabulary (PHP
== "1"pattern). Before this PR, the codebase had three divergent truthy parsers and several schema fields miscategorised asint. The reporter's crash (#558) exposed one of them; the others were latent. The right fix is type-level: rename the mistake rather than wrap a coercion around it.CommonDevice(pkg/model) is the documented public surface per NATS-144 (commit33ccf82). Rawpkg/schema/opnsense/pkg/schema/pfsensetypes can change without semver implications for external consumers who useCommonDevice.Type of Change
How Has This Been Tested?
just ci-check— all 4 lint blockers from round 3 resolvedgo test -race ./...— full suite including new regression fixturesTestSystem_LiberalBoolean_Issue558— exercises the reporter's exact scenario (<dnsallowoverride>on</dnsallowoverride>+ mixed vocab) end-to-end viacfgparser.NewXMLParser().ParseTestXMLParser_DecodeErrorIncludesElementPath— proves numeric-field malformed input still errors, but now with element-path contextTestBoolFlag_UnmarshalXML— full liberal vocabulary matrix including<tag>0</tag>→ false regression guardTestFlexBool_UnmarshalJSON_EscapeSequences— regression guard against the hand-stripped-quotes bug caught in review ("\u006fn"→ true)TestFlexInt_YAMLwith0x10,0o20,1_000,-5— verifies yaml.v3 native integer decoding (round 4 review finding)opndossier audit <config.xml>on OPNsense 26.1 — pending on userBreaking Changes
Schema layer only — not part of the public API surface per NATS-144.
Consumers who depend on the following face source-incompatible changes:
opnsense.Systemfields (10 fields, listed above) are nowBoolFlaginstead ofint. Comparisons likesys.RrdBackup == 1fail to compile; read asbool(sys.RrdBackup)orsys.RrdBackup.Bool().pfsense.Systemfields (3 fields) likewise migrated.opnsense.System/pfsense.System(not viaCommonDevice) now emitstrue/falsefor these fields instead of0/1.pfsense.isPfSenseValueTruedeleted — was unexported, so no external consumer was possible.CommonDeviceconsumers see no change (all migrated fields project to nativeboolin the converter).Dependencies
No dependency additions, upgrades, or removals.
encoding/jsonandgopkg.in/yaml.v3were already transitive dependencies.Risk Assessment
CommonDevice(public surface) unaffected.BoolFlag.UnmarshalXMLno longer treats<tag>0</tag>as true. Explicit regression guard added. Real fixtures using-1convention no longer report as "enabled" — see solution doc for rationale.CommonDeviceis the documented public surface per NATS-144.just ci-checkclean.Cross-Cutting Architecture
graph TD xml_decoder["encoding/xml decoder"] --> BoolFlag xml_decoder --> FlexBool xml_decoder --> FlexInt xml_decoder --> strict_int["strict int/bool"] BoolFlag -->|non-empty body| IsValueTrue FlexBool -->|all bodies| IsValueTrue FlexInt -->|string fallback| IsValueTrue IsValueTrue["shared.IsValueTrue / IsValueFalse"] BoolFlag --> converter["pkg/parser/*/converter*.go"] FlexBool --> converter FlexInt --> converter strict_int --> converter converter --> CommonDevice["common.CommonDevice (native bool/int)"] CommonDevice --> downstream["builder, reporters, plugins,<br/>JSON/YAML exports"] classDef layer fill:#e8f5e9,stroke:#2e7d32 classDef boundary fill:#fff3e0,stroke:#e65100 class BoolFlag,FlexBool,FlexInt,strict_int layer class CommonDevice boundaryBoundary rule: schema wrapper types never escape the schema layer. Converters extract the native Go primitive when projecting into
CommonDevice.Review Checklist
General
just lint0 issues)//nolint:directives on separate lines above calls (AGENTS.md §12.1 rule 6)%w(errors.Is/errors.Aswork throughWrapDecodeError)Schema / Type Design
xml.Marshaler/Unmarshaler+json.Marshaler/Unmarshaler+yaml.Marshaler/Unmarshaler)Tests
TestSystem_LiberalBoolean_Issue558)<tag>0</tag>→ false)Documentation
GOTCHAS.md§15 updated in same commit as interface changesdocs/development/standards.mdtype-selection guidance updateddocs/development/xml-structure-research.mdmigration caveat addeddocs/solutions/entry written for future maintainersResidual Actionable Work (tracked in
.context/compound-engineering/todos/)WrapDecodeErroroutput tocfgparser.ParseErrorso CLI exit code andJSONError.Details.element_pathare populated for automation consumers (addressesagent-native-reviewerW1/W2)./pfsense) because pfSense decodes in one pass. OPNsense-style section-by-section restructure would improve leaf-level accuracy.Additional Notes
WrapDecodeErrorelement-path annotation, (c) schema miscategorisation as a recurring bug class.docs/plans/2026-04-18-002-fix-issue-558-parser-on-value.md— implementation plan with requirements trace R1–R7, all met.<enable/>presence-based bug, March 2026) established the clean-fork architecture this PR extends. GOTCHAS §15.1 pointer-receiver pattern originated in that session.Closes #558