Skip to content

ignore unknown namespaces in ParseStrict for forward compatibility - #73

Merged
bborbe merged 1 commit into
masterfrom
feature/forward-compatible-namespaces
Aug 16, 2026
Merged

ignore unknown namespaces in ParseStrict for forward compatibility#73
bborbe merged 1 commit into
masterfrom
feature/forward-compatible-namespaces

Conversation

@bborbe

@bborbe bborbe commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Fixes a silent production wedge found today.

What broke

goUpdate: was added to MaintainerConfig in v0.48.1 and then written into two repos' .maintainer.yaml. The deployed github-releaser-agent still runs a binary built before that field existed, and it parses with ParseStrictKnownFields(true):

{
  "outcome": "failed",
  "error_category": "invalid_config",
  "invalid_value": "unmarshal .maintainer.yaml: yaml: unmarshal errors:\n  line 23: field goUpdate not found in type maintainerconfig.MaintainerConfig"
}

The failure cleared the task's assignee, so nothing retried it. Both repos simply stopped releasing — no tag, no alert, no retry. bborbe/github-update-go-watcher and bborbe/go-skeleton are still wedged.

Why the existing design didn't hold

The package doc said to add the field first, deploy the bot next, and treated the gap as a brief incompatible window. It isn't brief: it lasts until every consumer is rebuilt and redeployed. One schema is read by several independently-deployed binaries, so in practice any additive namespace is a fleet-wide breaking change — and it fails quietly, at the worst layer to debug.

The change

ParseStrict filters out top-level namespaces that MaintainerConfig doesn't declare before the strict decode, so:

  • unknown namespace (goUpdate: on an old binary) → ignored, parse succeeds
  • unknown key inside a known namespace (release.autoReleese) → still fatal

The known-namespace set is read from the struct's yaml tags by reflection, so adding a namespace remains a one-field edit with no second list to update.

Trade-off, stated plainly

A misspelled top-level namespace (prRevierer:) is now indistinguishable from one belonging to a newer bot, so it is ignored rather than fatal and the gate it meant to set stays false. That is a real loss. It is logged at WARNING to keep it discoverable, and pinned by a test so it reads as a decision rather than a regression. The alternative — keeping namespace typos fatal — is what caused today's outage.

Tests

  • ParseStrict ignores an unknown top-level namespace and still reads known ones
  • ParseStrict ignores the goUpdate namespace on a binary that predates it — the exact document that broke prod
  • ParseStrict still rejects a typo inside a known namespace — the property that must survive
  • inverted ParseStrict rejects typo in top-level prReviewer key to assert the new behaviour

make precommit green: 5 packages, 90%+ coverage, lint 0, vet clean, OSV 0, trivy 0.

Not fixed by this PR

Merging this does not unwedge anything on its own — github-releaser-agent carries its own compiled copy and must be rebuilt against the new version and redeployed. The two stuck release tasks also need re-driving, since a cleared assignee never retries.

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No ctx.Done() checks in the maintainerconfig package — consistent with the fact that neither new loop is a long-running async operation. The mechanical SHOULD findings for go-context/cancel-check-in-loop are inapplicable here (both loops are bounded map/reflect iterations with no async work). The glog import is pre-existing (project was using glog before this change). The ParseStrictDefault pairing is exempt per the rule's own criteria (internal-only parsing, never exposed). The test suite note is pre-existing.


PR Review: ignore unknown namespaces in ParseStrict for forward compatibility

Must Fix (Critical)

None.

Should Fix (Important)

None. The mechanical SHOULD findings for go-context/cancel-check-in-loop at lines 206 and 234 are inapplicable — both loops are bounded iterations over a small fixed set (yaml map keys and struct fields) with no async operations; ctx cancellation is irrelevant. The go-cli/slog-not-glog-in-new-projects finding is exempt (pre-existing glog usage, per the rule's own exemption clause). The go-parse/paired-parse-and-parsedefault finding is exempt (ParseStrict is internal-only, per the rule's own exemption clause).

Nice to Have (Optional)

None.


Traceability Report (selector mode)

  • Candidates: unable to compute — rules/index.json is not accessible in this session (blocked by sandbox). Step 4b-i glob filter could not run. Judgment-tier rules were not evaluated.
  • Mechanical funnel: 6 findings from go-context-assistant, go-quality-assistant, go-test-quality-assistant — all found to be inapplicable upon direct review or exempt (see above).
  • Applicable judgment rules: none evaluated (rules index inaccessible)

Manual Review Notes

The implementation is sound. Key decisions verified:

  • dropUnknownNamespaces round-trip fidelity: yaml.Unmarshal into map[string]yaml.Node → delete unknown keys → yaml.Marshal preserves nested content verbatim. Handles all scalar types, multi-document YAML, and anchors correctly since yaml.Node carries full AST, not just values.
  • knownNamespaces() with inline yaml tags: strings.Cut(tag, ",") correctly extracts the name from tags like yaml:"name,flow" — the flow bit is discarded intentionally.
  • glog usage: The namespace name logged at WARNING is a bot identifier (e.g., build-fix), not sensitive data. glog is pre-existing in the package.
  • Test coverage: New tests cover the exact prod failure scenario (goUpdate: added to two repos), the namespace-vs-typo-inside-namespace distinction, and the accepted cost (namespace typos now silently ignored). No gaps.
  • CHANGELOG: Correctly describes the behavioral change, the prod failure it fixes, and the known trade-off.
  • PR plan concerns: All five concerns addressed — round-trip fidelity, yaml tag handling, glog logging, performance (one-shot release path), and the namespace-typo trade-off are all handled correctly.

{
  "verdict": "approve",
  "summary": "The implementation is correct and well-documented. ParseStrict now filters unknown namespaces before strict decode, preserving the property that typos inside known namespaces fail loudly while unknown namespaces are tolerated for forward compatibility. The exact prod failure (goUpdate: wedging github-releaser-agent) is fixed. All PR-plan concerns were verified as addressed. The mechanical SHOULD findings for ctx.Done() in loops are inapplicable (both loops are bounded, no async work); glog and ParseStrictDefault findings are exempt per their own exemption clauses.",
  "comments": [],
  "concerns_addressed": [
    "correctness: dropUnknownNamespaces uses yaml.Node for round-trip fidelity — verified",
    "correctness: knownNamespaces() uses strings.Cut to handle inline yaml tags — verified",
    "correctness: glog logs namespace names (non-sensitive), pre-existing pattern — exempt",
    "performance: ParseStrict extra marshal/unmarshal is one-shot at release planning time — acceptable",
    "correctness: namespace-typo trade-off explicitly documented and tested — addressed"
  ]
}

@bborbe
bborbe merged commit 6891ac5 into master Aug 16, 2026
1 check passed
@bborbe
bborbe deleted the feature/forward-compatible-namespaces branch August 16, 2026 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant