Introduce structured FBC types: MajorMinor and Date

[joelanford]

Summary

• Replace plain string fields in FBC output types with structured MajorMinor and Date types that enforce format invariants at construction time
• Version.Name and Platform.Versions use MajorMinor (regex-validated, no leading zeros); Phase.StartDate/EndDate use *Date (nil = no date)
• Translation functions now return errors (collected via errors.Join) instead of silently passing through malformed data; empty and "N/A" timestamps translate to nil
• This enforces the FBC lifecycle contract — the schema that downstream consumers depend on — which is separate from the PLCC validation policy that governs data quality in the upstream source. PLCC validators check whether product lifecycle data meets content policy (tier requirements, date contiguity, release cadence, etc.); the FBC type layer ensures the output schema is well-formed by construction regardless of which PLCC validators are enabled.
• Reference test data regenerated: 146 → 34 packages (pipeline test bypasses PLCC validators, so the type layer now filters packages with formula timestamps, non-MAJOR.MINOR versions, etc.)

Test plan

• make test passes
• All type parsing tests (valid/invalid inputs, JSON round-trip)
• Pipeline tests verify bad versions/timestamps are rejected
• Writer tests confirm JSON/YAML output format unchanged
• cmd tests pass with original main_test.go unchanged

Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:08 PM UTC · Completed 8:23 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

Review — Approve

PR: Introduce structured FBC types: MajorMinor and Date

Summary

This PR replaces plain string fields in FBC output types with structured MajorMinor and Date types that enforce format invariants at construction time. The change shifts the translation layer from lenient (silently passing through malformed data) to strict (returning errors for invalid versions and timestamps), while preserving the existing output format via custom JSON marshalers that produce the same wire-format strings.

Correctness

Type safety and nil-dereference safety are sound. In translateVersion, if ParseMajorMinor(v.Name) fails, name is nil, but errs is non-empty, so the function returns before the *name dereference on line ~265. This pattern is used consistently throughout.

Error aggregation works correctly. newPackage collects per-version errors via errors.Join, and Translate unwraps them via unwrapJoined into individual ValidationResult.Reasons entries. The single level of unwrapping at Translate is sufficient since newPackage wraps each version's joined error in a fmt.Errorf, producing correctly-prefixed reason strings like version "1.0": phase "GA": start date: invalid timestamp "foo".

FilterIncompletePhases correctly adapted. The filter now checks ph.StartDate != nil && ph.EndDate != nil instead of comparing against empty strings, which is the correct nil-check for *Date pointer fields.

Sorting migrated correctly. The compareMajorMinor string-parsing function is replaced by MajorMinor.Compare, which works on parsed uint64 fields directly — more robust and semantically clear. slices.SortFunc is an appropriate modern replacement for sort.Slice.

Schema and serialization

JSON output format is preserved. MajorMinor.MarshalJSON produces "4.12" and Date.MarshalJSON produces "2025-01-01" — identical to the previous string-based serialization. The JSON round-trip tests in types_test.go verify this.

YAML output works via JSON round-tripping. Since sigs.k8s.io/yaml (v1.6.0) marshals Go types to JSON first then converts to YAML, the custom json.Marshaler implementations on MajorMinor and Date are sufficient — no separate YAML marshaler is needed. The version string "1.0" is correctly quoted in YAML output to prevent interpretation as a float.

omitempty on *Date fields causes nil dates to be omitted from output rather than serialized as empty strings. This is correct because FilterIncompletePhases removes phases with nil dates before serialization, so in normal operation both dates are always present in output. The omitempty tag is a safety net that avoids emitting null values if the filter pipeline were bypassed.

Test coverage

Tests are comprehensive and well-structured:

• types_test.go (new, 259 lines): covers parsing valid/invalid MajorMinor and Date values, string formatting, comparison, JSON marshal/unmarshal round-trips, nil *Date serialization, and invalid JSON input rejection
• fbc_test.go: split the old TestNewPackageUnparseableTimestamp into three focused tests (TestNewPackageNATimestamp, TestNewPackageUnparseableTimestamp, TestNewPackageEmptyTimestamp) plus a new TestNewPackageInvalidVersionName and TestTranslateVersionInvalidOCP
• pipeline_test.go: updated to exercise the new rejection path (bad-pkg now correctly rejected) and the filter path (empty/N/A dates → nil → phase stripped)
• writer_test.go: samplePackage helper updated to use structured types, expected output strings unchanged (validating format preservation)
• Reference files regenerated (146→34 packages) reflecting the stricter type-level validation

Design

The architectural separation is clean: PLCC validators enforce data quality policy (content requirements like date contiguity, tier rules), while the FBC type layer enforces structural schema invariants (version format, date format). These are genuinely independent concerns — a product can pass PLCC validation but have an unparseable timestamp format in a formula cell, and vice versa.

The panic("programmer error") guard in ParseMajorMinor is acceptable as a defensive check against regex capture-group miscounts that should never occur in practice.

No issues found

No correctness, security, or style issues were identified that require changes.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• AGENTS.md

Previous run

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, a protected governance file. No linked issue provides authorization for this change. Human approval is required for all protected-path changes regardless of context.
  Remediation: Link to an issue that authorizes modifications to AGENTS.md, or obtain explicit maintainer approval for the protected-path change.

Medium

• [architecture-shift-undocumented] pkg/fbc/doc.go:20 — The PR shifts the translation philosophy from "lenient parsing" to "strict translation." While doc.go and AGENTS.md are updated, no formal design document captures the rationale, alternatives considered, or implications for the two-layer validation architecture.
  Remediation: Consider documenting the design rationale more formally if the project adopts ADRs.

• [breaking-api-change] pkg/fbc/fbc.go:42 — Exported type changes (Version.Name: string→MajorMinor, Phase.StartDate/EndDate: string→*Date, Platform.Versions: []string→[]MajorMinor) are Go-level breaking changes. The module has no git tags or releases (pre-1.0), limiting external impact, but downstream Go importers would break at compile time.
  Remediation: Acknowledge this as a pre-1.0 breaking change in the PR description.

• [breaking-json-output] pkg/fbc/fbc.go:137 — Phase.StartDate and Phase.EndDate now use omitempty JSON tags. Nil dates are omitted from JSON output rather than appearing as empty strings. In practice, FilterIncompletePhases strips nil-date phases by default, so this mainly affects non-default filter pipelines.
  Remediation: Remove omitempty from Phase date JSON tags to maintain backward compatibility, or document this as a known change.

• [breaking-behavior] pkg/fbc/fbc.go:156 — Translate() now rejects products with invalid version names (non-MAJOR.MINOR) or unparseable timestamps, whereas previously these passed through with degraded data. This is intentional per the PR description and redundant with existing PLCC validators in the normal pipeline.
  Remediation: Document this behavior change and its interaction with existing PLCC validators.

• [stale-doc] docs/FBC_SCHEMA.md:45 — Phase fields startDate/endDate are documented as Required=yes but are now optional (omitempty). The schema overview does not reflect that these fields can be absent.
  Remediation: Update docs/FBC_SCHEMA.md: change Phase date fields from Required=yes to Required=no.

Low

• [missing-authorization] — Non-trivial structural change with no linked issue. The PR has a clear description and "enhancement" label, but formal issue-based authorization is missing.
• [scope-creep] Makefile:13 — Unrelated addition of -count 1 (disables test caching). Plausibly related to test data regeneration but not documented.
• [testdata-regeneration-unexplained] pkg/fbc/testdata/reference-fbc.yaml — Reference test data reduced from 146→34 packages. The PR description acknowledges this but doesn't break down rejection reasons.
• [error-handling] pkg/fbc/fbc.go:159 — Nested errors.Join in newPackage produces multi-line entries in ValidationResult.Reasons, which may render poorly in structured JSON logs.
• [panic-usage] pkg/fbc/types.go:63 — ParseMajorMinor includes a panic for an unreachable code path. The codebase has no other panic calls in production code.
• [naming-inconsistency] pkg/fbc/types.go:51 — MajorMinor uses uint64 for Major/Minor fields; this is the default return type of strconv.ParseUint but oversized for version numbers.

Previous run (2)

Review

Findings

High

• [breaking-api] pkg/fbc/fbc.go — Exported struct field types changed: Version.Name (string → MajorMinor), Phase.StartDate/EndDate (string → *Date), Platform.Versions ([]string → []MajorMinor). Any external Go code importing the fbc package will break at compile time. The JSON wire format is preserved (custom marshalers produce the same strings), but the Go API surface is broken. For a v0 module this is permitted by Go module semantics, but the change should be documented as breaking.
  Remediation: Document as a breaking change in the PR description or release notes. If external consumers exist, consider a deprecation path or accessor methods.

• [stale-doc] AGENTS.md — Two factually incorrect statements after this PR: (1) line 133 states "N/A or empty timestamps translate to empty strings" — they now translate to nil. (2) Line 142 states "newPackage() silently converts unparseable timestamps to empty strings" — newPackage now returns errors for invalid data.
  Remediation: Update both references to reflect nil semantics and error-returning behavior.

• [stale-doc] pkg/fbc/doc.go — Package documentation states "newPackage translates a PLCC product without failing on bad data (unparseable timestamps become empty strings)". This is now incorrect — newPackage returns errors for invalid data.
  Remediation: Update doc.go to describe the new strict translation behavior.

Medium

• [missing-authorization] — Non-trivial architectural change (new type system, 2 new files, error handling model change, test data regeneration) without a linked issue. The PR body provides good context, but a linked issue aids traceability for foundational changes.
  See also: [architectural-coherence] finding below.

• [architectural-coherence] pkg/fbc/fbc.go — Adding error returns to translation functions changes the documented design pattern. AGENTS.md, CLAUDE.md, and doc.go all describe a "lenient parsing" model where the FBC layer silently converts bad data and the PLCC layer handles validation. This PR intentionally shifts to strict validation at the FBC type layer, which is a reasonable design evolution, but the documentation must be updated to reflect it.
  See also: [stale-doc] findings for AGENTS.md and doc.go.

• [backward-incompatible] pkg/fbc/fbc.go — Translate() now returns validation failures for packages with invalid versions or timestamps that were previously silently accepted. The PR body documents this, but downstream callers relying on lenient behavior may be affected.

• [breaking-schema] pkg/fbc/types.go — A nil *Date marshals as JSON null, whereas the previous behavior produced an empty string "". FilterIncompletePhases drops phases with nil dates before serialization in the default pipeline, but callers using custom filter pipelines could encounter null in date fields where strings were expected.

• [stale-doc] docs/FBC_SCHEMA.md — Documents output field types as string for version names and dates. The JSON wire format is unchanged (custom marshalers produce strings), so the schema documentation is correct from a consumer perspective. However, noting the Go type change would help developers working with the package.

Low

• [error-handling-gap] pkg/fbc/types.go — ParseMajorMinor discards strconv.ParseUint errors. The regex validates digit format but not magnitude; a version component exceeding 2^64-1 would silently corrupt to math.MaxUint64. Extremely unlikely in practice but technically unsound.

• [scope-clarity] pkg/fbc/testdata/* — Reference test data reduced from 146 to 34 packages. The PR body explains this ("pipeline test bypasses PLCC validators, so the type layer now filters packages with formula timestamps, non-MAJOR.MINOR versions, etc."), but the explanation is easy to miss.

• [sorting-conventions] pkg/fbc/fbc.go — sort.Slice replaced with slices.SortFunc, while pkg/plcc/plcc.go still uses sort.Slice. Minor inconsistency during incremental modernization.

• [stale-doc] docs/VALIDATION_RULES.md — References "empty" dates instead of "nil" in FilterIncompletePhases description and summary table.

• [missing-doc] AGENTS.md — Key Types section does not list the new exported types MajorMinor and Date.

• [stale-doc] AGENTS.md — Documents make test as go test -v ./... but the PR changes it to go test -v -count 1 ./....

---

Labels: PR introduces new structured types and changes exported API surface, qualifying as an enhancement.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:55 PM UTC · Completed 9:09 PM UTC
Commit: feb9bda · View workflow run →

[fgiudici]

(rebased on main)

[asergienk]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:24 PM UTC · Completed 5:29 PM UTC
Commit: 7128ea4 · View workflow run →

Superseded by updated review

[fgiudici]

This PR adds a second filtering layer during FBC conversion:

1. PLCC validation
2. FBC conversion + type enforcement (new)

now step 2) may fail.

This is causing the current tests with the reference PLCC data on the --split value to fail as with PR 27 we relaxed the filters on OCP versions MAJOR.MINOR for older operators and we are now failing during FBC conversion.
I would say this is a real failure: i.e., we want to enforce syntax here and the PLCC filters should enforce and catch this (this is syntax based). Otherwise we have to adapt the tests and expect this failure to happen (so that we are sure FBC catches it).

We have to drop the panic() before merging.
We may also consider using direct Date types instead of pointers.

[fgiudici]

We are enforcing types now in FBC.
Why don't we drop the pointer here and the "omitempty" as well?
We don't really have to disambiguate date "0" and no date and we are going to enforce we have dates for the phases. We are going to enforce the type, using a Date type here would be more straightforward.

[fgiudici]

Previously, the newPackage could not fail.
Now we are introducing FBC side checking enforcing types.
This is an error that should be reported as an error.
In this function, the FBC filters were mutating ones only, so there was only the need to log the mutation.
Now we are enforcing filters and we can error, we must report back errors.

[fgiudici]

nit: why uint64 and not just uint?

[fgiudici]

This is a library, should not panic!