From 2b19a121fa47f416397c44023e09a8ca8c242d84 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 2 Aug 2026 22:53:00 +0000 Subject: [PATCH] Generalize NWS forecast discussion heading parsing --- docs/roadmap/afd-section-heading-variants.md | 240 ++++++----- docs/roadmap/implementation.md | 397 +++++++++++------- internal/providers/nws/forecast_discussion.go | 142 +++++-- .../providers/nws/forecast_discussion_test.go | 170 ++++++-- 4 files changed, 618 insertions(+), 331 deletions(-) diff --git a/docs/roadmap/afd-section-heading-variants.md b/docs/roadmap/afd-section-heading-variants.md index abe882d..d323c35 100644 --- a/docs/roadmap/afd-section-heading-variants.md +++ b/docs/roadmap/afd-section-heading-variants.md @@ -1,141 +1,171 @@ -# NWS AFD Section Heading Variants +# NWS AFD Section Parsing Resilience ## Status -Implemented. +The original ellipsis-first and slash-qualified heading feature is implemented. +The resilience follow-up defined below is proposed and unimplemented. -## Problem +## Completed Baseline -The NWS Area Forecast Discussion parser recognizes supported section headings -only when the section name is followed immediately by an ellipsis, for example: - -```text -.SHORT TERM... (Through Monday) -.LONG TERM... (Tuesday through Sunday) -.AVIATION... (For the 18z TAFs through 18z Monday) -``` - -NWS offices also publish slash-qualified headings: - -```text -.SHORT TERM /THROUGH MONDAY/... -.LONG TERM /MONDAY NIGHT THROUGH SUNDAY/... -.AVIATION /18Z TAFS THROUGH 18Z MONDAY/... -``` - -The current exact-prefix discovery logic does not find slash-qualified short- -or long-term sections. Its boundary detection also does not recognize a -slash-qualified aviation heading, so preceding section text can absorb aviation -content. The forecast discussion can still normalize successfully, leaving -downstream consumers with missing or incorrectly bounded structured prose. - -## Feature Objective - -Make supported AFD section recognition consistent across section discovery, -section-boundary detection, and qualifier extraction while preserving the -existing canonical forecast-discussion contract. - -## Targeted End State - -One NWS-specific heading parser classifies a trimmed AFD line as either a -supported heading or a non-heading. For a supported heading it provides: - -- the canonical section identity; -- the qualifier, if present; and -- enough information for discovery and boundary detection to use the same - recognition result. - -The supported section identities remain: - -- `KEY MESSAGES`; -- `SHORT TERM`; -- `LONG TERM`; and -- `AVIATION`. - -`KEY MESSAGES`, `SHORT TERM`, and `LONG TERM` participate in the existing -canonical extraction behavior. `AVIATION` remains a recognized boundary only; -it does not become a canonical payload field. - -For each supported identity, the parser accepts both heading families: +The NWS Area Forecast Discussion parser now recognizes these heading families +for key messages, short term, long term, and aviation: ```text .
... .
//... ``` -Qualifier handling is deterministic: +Discovery, qualifier extraction, and recognized-section boundary detection use +one provider-specific parser. Slash delimiters are removed from qualifiers, +legacy qualifier text is preserved, and aviation remains a boundary rather than +a canonical field. Focused provider and normalizer regressions cover the +implemented forms. -- leading and trailing whitespace is removed; -- the enclosing slash pair used by the slash-qualified form is removed; -- qualifier content otherwise retains its published text, including - parentheses in the existing ellipsis-first form; and -- a heading without a qualifier yields an empty qualifier. +## Remaining Problem -The same classification rules govern discovery and termination. A recognized -heading ends the preceding section without becoming part of its text. Unknown -section names, unrelated dotted lines, and malformed slash-qualified lines are -not treated as supported headings. +Current NWS bulletins vary beyond those two same-line forms. In particular: -## Scope of Work +- an ellipsis-first heading may put its qualifier on the next nonblank line; +- NWS presentation output can place `-- Changed Discussion --` markers and an + `Updated at` line around section content; +- valid AFDs can include `DISCUSSION`, `UPDATE`, `MARINE`, `HYDROLOGY`, + `CLIMATE`, `FIRE WEATHER`, office watch/advisory blocks, and other topic + headings; and +- minor punctuation or qualifier changes can produce headings that are + structurally valid but absent from the parser's identity whitelist. -The feature includes: +The current parser recognizes only four exact identities. Unknown headings do +not terminate the preceding section. It also expects a text section's optional +qualifier and `Issued at` line in a narrow order. Consequently, valid upstream +format variants can leave qualifier and section-time fields empty, leak +presentation metadata into prose or key messages, or allow one section to +absorb another. -- consolidating supported-heading recognition and qualifier extraction in - `internal/providers/nws`; -- applying that recognition consistently to section lookup and section - boundaries; -- preserving key-message parsing and short- and long-term section mapping; -- covering legacy, slash-qualified, mixed-format, and malformed headings with - focused provider-parser tests; and -- verifying that parsed short- and long-term sections continue through the NWS - normalizer into the existing canonical payload. +## Feature Objective -Existing ellipsis-first fixtures and behavior remain regression coverage. New -fixtures should be minimal and representative rather than copies of complete -upstream bulletins unless a full bulletin is needed to prove an interaction. +Make AFD parsing resilient to minor upstream format evolution by separating: + +1. generic structural heading recognition; +2. section boundary scanning; +3. canonical section-role selection; and +4. section-preamble and presentation cleanup. + +The parser should accept new structurally valid topic names as safe boundaries +without treating them as new canonical fields. + +## Targeted End State + +### Generic heading recognition + +One NWS-specific heading parser recognizes structurally valid uppercase AFD +topic lines independently of the canonical model. It supports: + +```text +.... +. //... +``` + +The identity may contain uppercase ASCII letters, digits, horizontal whitespace, +`/`, `&`, apostrophes, and hyphens. Outer whitespace and whitespace immediately +before the ellipsis are ignored, and internal identity whitespace is collapsed +to one space for role lookup. Slash-qualified headings require a nonempty +qualifier but may contain slash characters inside that qualifier. Lowercase +prose, ordinary dotted lines, and malformed delimiters remain non-headings. + +Every structurally valid heading terminates the preceding section. This applies +to known boundary-only sections such as aviation and to future or office-specific +topic identities that weatherfeeder does not map. + +### Canonical role selection + +A single provider-local role registry maps only these identities into existing +parsed fields: + +- `KEY MESSAGES` to key messages; +- `SHORT TERM` to the short-term section; and +- `LONG TERM` to the long-term section. + +All other identities are boundary-only. The first occurrence of each mapped +identity wins, preserving current behavior if an unusual bulletin repeats a +section. Heading syntax and canonical roles do not duplicate identity lists. + +### Section scanning + +The discussion text is scanned once into ordered section blocks. A new heading, +`&&`, `$$`, or the existing watch/advisory terminator ends the active block. +Original body lines are preserved until provider presentation cleanup is +applied. Preamble text before the first heading and post-`$$` signatures are not +treated as section content. + +### Preamble and presentation handling + +For short- and long-term sections: + +- a qualifier on the heading line remains authoritative; +- when the heading has no qualifier, a standalone parenthesized first content + line becomes the qualifier and retains its parentheses; +- an optional case-insensitive `Issued at` line after the qualifier is parsed + into the existing section issue time; and +- exact NWS change-presentation marker lines are removed without removing + arbitrary dashed prose. + +For key messages, exact change-presentation markers and one leading `Issued at` +or `Updated at` metadata line are removed before bullet parsing. Those metadata +lines never become key messages. + +### Representative coverage + +Tests include small structural tables and maintained local HTML fixtures for at +least two real NWS formatting families: slash-qualified same-line headings and +ellipsis-first headings with multiline qualifiers or change-presentation +markers. Tests remain deterministic and never contact live services. ## Compatibility and Contracts -This is a provider-parsing compatibility improvement. It does not change: +This remains a provider-parsing compatibility improvement. It does not change: - event kinds or raw and canonical schema identifiers; - canonical models or JSON field names; -- source configuration or polling behavior; +- source configuration, URLs, or polling behavior; - event envelope or effective-time behavior; - Postgres tables or event-to-row mapping; or - downstream sink and consumer responsibilities. -Current-behavior documentation should be updated only if implementation reveals -an externally observable contract change beyond the scope defined here. +A single `DISCUSSION` section and other currently unmapped identities are +recognized as boundaries but are not forced into short- or long-term fields. +Exposing such content would require a separate canonical schema decision. ## Acceptance Criteria -The feature is complete when automated tests demonstrate that: +The resilience follow-up is complete when automated tests demonstrate that: -- all currently accepted ellipsis-first headings produce unchanged results; -- slash-qualified short- and long-term headings are discovered and their - qualifiers exclude slash delimiters; -- slash-qualified recognized headings correctly terminate a preceding section, - including an aviation heading following a long-term section; -- documents mixing the two heading families are parsed correctly; -- headings without qualifiers retain existing behavior; -- unknown or malformed heading-like lines do not create supported sections or - prematurely terminate one; -- the NWS forecast-discussion normalizer emits populated canonical short- and - long-term fields for representative slash-qualified input without adding an - aviation field; and -- the full repository test suite passes. +- every previously accepted heading and canonical result remains compatible; +- generic structurally valid identities terminate preceding content without + becoming canonical fields; +- malformed heading-like lines and lowercase prose remain body content; +- identities containing `/` and slash qualifiers containing `/` are parsed + without ambiguity; +- next-line parenthesized qualifiers and following `Issued at` lines populate + the existing short- and long-term metadata correctly; +- exact NWS change markers and leading key-message timestamps do not leak into + canonical prose or messages; +- repeated mapped sections preserve first-occurrence behavior; +- genuine cross-office fixture styles propagate correctly through the NWS + normalizer with unchanged wire shape; and +- focused tests, the full repository suite, and static analysis pass. ## Non-Goals -This feature does not: +This follow-up does not: -- add new canonical forecast-discussion sections; -- add aviation prose to the canonical model; -- recognize arbitrary or previously unsupported NWS section families; -- introduce heuristic section-name matching or AFD summarization; -- preserve complete raw AFD documents in canonical payloads; or -- change schemas, persistence contracts, configuration, or downstream APIs. +- add canonical `discussion`, aviation, marine, hydrology, climate, fire + weather, update, or arbitrary-section fields; +- infer short- or long-term semantics from an unknown heading; +- accept free-form or lowercase prose as a heading; +- introduce heuristic summarization; +- preserve complete raw AFD documents in canonical payloads; +- change schemas, persistence contracts, configuration, or downstream APIs; or +- fetch live NWS data during tests. -Support for additional section identities or broader AFD structure should be -driven by a separate consumer requirement and roadmap. +Canonical support for additional AFD section identities should be driven by a +separate consumer requirement and schema roadmap. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index ca7522d..a8cb9a1 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,181 +1,264 @@ -# NWS AFD Section Heading Variants Implementation Plan +# NWS AFD Section Parsing Resilience Implementation Plan ## Purpose -Implement the end state defined in -[`afd-section-heading-variants.md`](afd-section-heading-variants.md): recognize -legacy ellipsis-first and slash-qualified NWS Area Forecast Discussion headings -consistently during section discovery, boundary detection, and qualifier -extraction without changing the canonical event contract. - -Complete the stages below in order. Keep each stage limited to the files and -behavior it names, and leave the repository passing its tests before proceeding. +Complete the resilience end state in +[`afd-section-heading-variants.md`](afd-section-heading-variants.md) while +preserving the current canonical forecast-discussion contract. Stages 1-3 below +summarize completed work. Implement Stages 4-8 in order. ## Cross-Stage Constraints -- Keep all provider-format parsing in `internal/providers/nws`. -- Use only the Go standard library; do not add a dependency. -- Do not change `model`, `standards`, source configuration, polling behavior, - schemas, event-envelope behavior, Postgres mapping, or sink behavior. -- Preserve the existing case-sensitive supported section identities: `KEY - MESSAGES`, `SHORT TERM`, `LONG TERM`, and `AVIATION`. -- Continue exposing only key messages, short term, and long term in the parsed - and canonical forecast-discussion payloads. `AVIATION` is a boundary marker, - not a new output field. -- Preserve source body lines for the existing prose parsers; heading - normalization must not alter section text, issue-time parsing, signature - trimming, or paragraph joining. -- Treat unknown section names and malformed slash-qualified lines as ordinary - body lines, not as recognized boundaries. -- Use deterministic unit tests and the checked-in fixture. Do not contact live - NWS services. +- Keep all production parsing changes in + `internal/providers/nws/forecast_discussion.go`. +- Use only the Go standard library and keep helpers unexported. +- Do not change `model`, `standards`, source configuration or polling, schemas, + event-envelope behavior, Postgres mapping, sinks, consumer docs, or current + integration contracts. +- Continue exposing only key messages, short term, and long term. All other + structurally valid identities are boundary-only. +- Preserve first-occurrence behavior for mapped sections. +- Preserve raw body lines during structural scanning; apply provider-specific + cleanup only when parsing a block's content. +- Keep parsing deterministic. Tests must use local strings and fixtures, never + live NWS requests. +- Preserve all pre-existing user work and avoid unrelated refactors. -## Stage 1: Centralize Heading Parsing and Section Extraction +## Stage 1: Centralize Known Heading Parsing — Completed -Implement the provider-level parsing primitive and make it the sole source of -heading identity and qualifier interpretation. +The provider parser gained one heading classifier, explicit section and block +types, and shared discovery, boundary, and qualifier handling for the original +four identities. Key-message and text-section consumers were moved to the block +representation, and immediately adjacent recognized headings became valid +boundaries. -1. In `internal/providers/nws/forecast_discussion.go`, define unexported string - constants for the four supported section identities. Use those constants in - `ParseForecastDiscussionText` instead of repeating string literals. -2. Add an unexported `forecastDiscussionSectionHeading` value with `section` - and `qualifier` fields, plus a - `parseForecastDiscussionSectionHeading(string) (forecastDiscussionSectionHeading, bool)` - helper. The helper must trim outer line whitespace and implement exactly - these two case-sensitive, whole-line grammars: +## Stage 2: Add Heading-Variant Regressions — Completed - ```text - ^\.(KEY MESSAGES|SHORT TERM|LONG TERM|AVIATION)\.\.\.(.*)$ - ^\.(KEY MESSAGES|SHORT TERM|LONG TERM|AVIATION)[ \t]+/([^/]+)/\.\.\.$ - ``` +Provider tests now cover ellipsis-first and slash-qualified headings, malformed +forms, empty bodies, mixed heading families, and slash-qualified aviation +termination. Normalizer coverage verifies canonical propagation and unchanged +wire shape. - Keep the supported-identity alternation in one shared pattern constant used - to build both regular expressions, so the accepted identity list cannot - drift between the two forms. -3. For the ellipsis-first form, trim leading and trailing whitespace from the - text following the ellipsis and otherwise preserve it verbatim. This retains - current results such as `(Through Late Sunday Night)` and permits an empty - qualifier. -4. For the slash-qualified form, require at least one space or tab before the - opening slash, forbid embedded slash characters, require the closing slash - immediately before the final ellipsis, and reject a qualifier that becomes - empty after trimming. Return the trimmed inner text without either slash. - Do not case-fold identities or accept trailing text after the final - ellipsis. -5. Add an unexported `forecastDiscussionSectionBlock` containing the parsed - heading and its body lines. Change `extractForecastDiscussionSection` to - return this value. Find the requested section by calling the new heading - parser and comparing its `section` field to the requested identity; remove - the current constructed exact-prefix lookup. -6. While collecting a block body, retain the existing terminators `&&`, `$$`, - and lines containing `WATCHES/WARNINGS/ADVISORIES`. Also stop before every - subsequent line recognized by the new heading parser, including a heading - immediately following the current heading. Remove the existing - `j > i+1` exception so an empty section cannot absorb the next heading. - Preserve original, untrimmed body lines in the returned block. -7. Update the consumers of the extracted block: +## Stage 3: Validate and Record the Baseline — Completed - - pass only `block.body` to key-message parsing and change - `parseForecastDiscussionKeyMessages` so it no longer assumes the heading - occupies element zero; - - make `parseForecastDiscussionTextSection` consume the block, initialize - `Qualifier` directly from `block.heading.qualifier`, and process - `block.body` with the existing issue-time and prose logic; and - - remove `forecastDiscussionHeaderRE`, - `isForecastDiscussionSectionHeader`, and - `parseForecastDiscussionQualifier` after all callers use the centralized - helper. -8. In `internal/providers/nws/forecast_discussion_test.go`, add a table-driven - unit test for `parseForecastDiscussionSectionHeading` with, at minimum: +Focused and full tests passed, production changes remained inside the NWS +provider parser, and the original feature roadmap was marked implemented. - - ellipsis-first headings for all four identities; - - an ellipsis-first heading with no qualifier; - - slash-qualified headings for all four identities; - - leading/trailing outer whitespace and multiple spaces before a slash; - - exact qualifier expectations showing that legacy parentheses remain and - slash delimiters are removed; and - - negative cases for an unknown identity, missing opening or closing slash, - an embedded slash, a whitespace-only slash qualifier, missing required - whitespace before the slash, missing final ellipsis, trailing text after a - slash-qualified heading, and a lowercase section identity. +## Stage 4: Generalize Heading Syntax and Centralize Canonical Roles + +Decouple structural heading recognition from canonical section selection. + +1. In `internal/providers/nws/forecast_discussion.go`, replace the four-name + regular-expression alternation with a generic heading parser. Keep the + `forecastDiscussionSectionHeading` result, with normalized `section` and + `qualifier` fields. +2. Implement the generic parser with these decisions: + + - trim outer whitespace and require a leading `.`; + - try the slash-qualified form before the legacy ellipsis-first form so the + terminal ellipsis of an all-uppercase slash heading cannot be mistaken for + a legacy identity; + - accept identities made only from uppercase ASCII letters, digits, + horizontal whitespace, `/`, `&`, apostrophes, and hyphens, with at least + one letter or digit; + - trim the identity and collapse every internal run of spaces or tabs to one + ASCII space before returning it or performing role lookup; + - accept optional horizontal whitespace between the identity and the legacy + `...` delimiter; + - for legacy headings, store trimmed text after the first `...` as the + qualifier, preserving parentheses and permitting an empty value; + - for slash-qualified headings, remove the terminal `/...`, then use the + first slash preceded by horizontal whitespace as the opening separator; + trim the identity before that separator and the qualifier after it, reject + an empty qualifier, and allow additional `/` characters inside the + qualifier; slashes inside an identity must therefore be adjacent to its + other identity characters rather than preceded by whitespace; + - reject trailing text after a slash-qualified terminal, lowercase or mixed + case identities, missing delimiters, and lines whose identity contains + other punctuation; and + - keep ASCII `...` as the only delimiter because that is the raw product + convention; do not interpret a Unicode ellipsis. +3. Replace the duplicated identity constants and identity-pattern string with: + + - an unexported `forecastDiscussionSectionRole` enum for key messages, short + term, and long term; and + - one `map[string]forecastDiscussionSectionRole` containing exactly `KEY + MESSAGES`, `SHORT TERM`, and `LONG TERM`. + + Unknown identities and `AVIATION` intentionally have no role entry; successful + structural parsing is sufficient for boundary behavior. +4. Update the heading table test in + `internal/providers/nws/forecast_discussion_test.go`. Preserve every legacy + positive case and revise the old policy-specific negatives: + + - `.SYNOPSIS...`, `.UPDATE...`, `.MARINE...`, `.HYDROLOGY...`, an office + watch/advisory heading, and `.PRELIMINARY POINT TEMPS/POPS ...` are valid + generic headings; + - a slash-qualified heading whose qualifier contains `/` is valid; + - leading/trailing whitespace, multiple separator spaces, repeated internal + identity whitespace, and whitespace before a legacy ellipsis are valid and + produce the normalized identity; and + - lowercase prose, an empty or punctuation-only identity, malformed slash + terminals, missing ellipses, and unsupported identity punctuation remain + invalid. +5. Add assertions that every role-registry key parses successfully, and confirm + that no production switch or second collection repeats the mapped identity + list. Run and pass: ```sh gofmt -w internal/providers/nws/forecast_discussion.go internal/providers/nws/forecast_discussion_test.go -go test ./internal/providers/nws +go test -count=1 ./internal/providers/nws ``` -Do not proceed until legacy provider tests still pass and the centralized -heading test covers every accepted identity in both forms. +Do not proceed until generic syntax tests pass without changing canonical +models or schemas. -## Stage 2: Add Provider and Normalizer Regression Coverage +## Stage 5: Scan Ordered Blocks Once and Use Generic Boundaries -Prove section discovery, boundary behavior, and canonical propagation using the -centralized parser. No production normalizer changes should be necessary. +Replace repeated per-identity extraction with one structural pass. -1. In `internal/providers/nws/forecast_discussion_test.go`, add focused tests of - `extractForecastDiscussionSection` using small line slices without `&&` - between sections. Cover: +1. Replace `extractForecastDiscussionSection` with + `parseForecastDiscussionSectionBlocks(lines []string) []forecastDiscussionSectionBlock`. + The scanner must: - - a slash-qualified `LONG TERM` block followed by a slash-qualified - `AVIATION` heading, asserting that the long-term qualifier is normalized, - only long-term prose is returned, and aviation heading/body text is - excluded; - - a recognized heading immediately following another recognized heading, - asserting that the first block has an empty body; and - - unknown and malformed heading-like lines inside a block, asserting that - they remain in the body and do not terminate it before a real terminator or - recognized heading. -2. Add a provider end-to-end regression based on - `testdata/forecast_discussion_sample.html`. Derive the input in the test by - replacing selected fixture headings rather than duplicating the full HTML - fixture. Guard each replacement with an assertion that its original heading - exists so fixture drift cannot make the test pass without exercising the new - syntax. -3. Make that derived bulletin intentionally mix heading families: retain the - legacy `KEY MESSAGES` heading, convert `SHORT TERM` and `LONG TERM` to - slash-qualified headings, and convert `AVIATION` to a slash-qualified - heading. Parse it through `ParseForecastDiscussionHTML` and assert: + - traverse lines once in source order; + - start a block on every structurally valid heading; + - finish the active block before a new heading; + - finish and clear the active block on `&&`; + - finish the active block and stop scanning on `$$`; + - retain the existing watch/advisory termination safeguard even though a + normal dotted watch/advisory heading is structurally recognized; + - ignore preamble lines before the first heading and signature lines after + `$$`; and + - append original, untrimmed body lines to the active block. +2. Refactor `ParseForecastDiscussionText` to iterate over the ordered blocks + once and look up each heading identity in the role registry. Ignore blocks + with no role. Populate each mapped output only if it has not already been + populated, so the first occurrence wins. Track seen roles explicitly rather + than inferring them from output values, because an empty first key-message + block still counts as the first occurrence. +3. Preserve contextual errors when a mapped text block fails to parse; include + the parsed heading identity in the wrapped error. +4. Replace extraction tests with scanner tests covering: - - key messages are unchanged; - - short- and long-term sections are non-nil; - - their qualifiers equal the inner slash text with no slash delimiters or - legacy parentheses; - - their existing issued times and representative prose remain intact; and - - long-term text contains no aviation heading or aviation prose. -4. In `internal/normalizers/nws/forecast_discussion_test.go`, add a regression - that derives the same mixed-format HTML from the shared fixture and passes it - through `ForecastDiscussionNormalizer.Normalize`. Assert the existing kind, - canonical schema, and effective time; exact normalized short- and long-term - qualifiers; representative section text; and absence of aviation content - from long-term text. -5. Marshal the slash-qualified normalizer result and assert that it has no - top-level `aviation` or generic `sections` key. Do not add or alter canonical - model fields to satisfy this test. + - consecutive headings and empty bodies; + - `&&`, `$$`, and watch/advisory termination; + - unknown valid headings terminating short- or long-term content even when + no `&&` is present; + - malformed heading-like lines remaining inside the active body; + - preamble and post-signature exclusion; + - ordered block retention; and + - duplicate mapped sections where the parser keeps the first canonical + occurrence. + +Run and pass: + +```sh +gofmt -w internal/providers/nws/forecast_discussion.go internal/providers/nws/forecast_discussion_test.go +go test -count=1 ./internal/providers/nws +``` + +## Stage 6: Normalize Multiline Preambles and NWS Presentation Markers + +Handle real section-layout variation without weakening heading syntax. + +1. Add an exact provider-local marker predicate for lines whose trimmed value is + `-- Changed Discussion --` or `-- End Changed Discussion --`, compared + case-insensitively. Add a helper that removes only those complete marker + lines from a block body. Do not remove arbitrary dashed lines or bullet text. +2. Apply marker removal before parsing both key-message and text-section bodies. +3. Refactor text-section preamble parsing in this exact order: + + - trim blank lines after marker removal; + - start with the qualifier parsed from the heading; + - only when that qualifier is empty, consume the first content line as a + qualifier if its trimmed value is a nonempty standalone parenthetical + string beginning with `(` and ending with `)`; preserve the parentheses; + - after the optional qualifier, consume an optional `Issued at` line and + parse it into `ForecastDiscussionSection.IssuedAt`; and + - pass only the remaining lines to existing signature trimming and paragraph + joining. +4. Make recognition of the `Issued at` label ASCII case-insensitive while + retaining the existing timestamp grammar and error behavior. The top-level + header path, which passes an unlabeled timestamp, must remain compatible. +5. Before key-message bullet parsing, remove markers, trim blank lines, and + discard at most one leading metadata line beginning case-insensitively with + `Issued at` or `Updated at`. Do not discard timestamp-like lines after the + first message begins. +6. Add focused tests for: + + - same-line legacy and slash qualifiers remaining unchanged; + - next-line parenthesized qualifiers followed by `Issued at`; + - uppercase `ISSUED AT`; + - a heading qualifier taking precedence over a following parenthetical prose + line; + - empty sections; + - invalid issue timestamps retaining contextual errors; + - marker removal at the beginning and end of text sections; + - key-message markers and a leading `Updated at` line not becoming messages; + and + - arbitrary dashed prose remaining content. + +Run and pass: + +```sh +gofmt -w internal/providers/nws/forecast_discussion.go internal/providers/nws/forecast_discussion_test.go +go test -count=1 ./internal/providers/nws +``` + +## Stage 7: Add Cross-Office Fixtures and Normalizer Regressions + +Prove the resilient parser against representative source shapes rather than only +synthetic heading replacement. + +1. Retain the existing LSX fixture and mixed-format test for regression + compatibility. +2. Add one compact, maintained HTML fixture under + `internal/providers/nws/testdata/` representing a second real NWS formatting + family. It must contain: + + - a valid AFD header and issue time; + - key-message change markers plus a leading `Updated at` line; + - ellipsis-first short- and long-term headings with qualifiers on the next + line; + - `Issued at` lines following those qualifiers; + - at least one structurally valid boundary-only section; and + - representative prose sufficient to detect metadata or adjacent-section + leakage. + + Keep the fixture concise; include no unrelated webpage content, secrets, or + private data. +3. Add a provider end-to-end test that parses the new fixture and asserts exact + key messages, qualifiers, section issue times, representative prose, and + absence of change markers, timestamp metadata, and boundary-only content. +4. Add a normalizer regression using the same fixture. Verify kind, canonical + schema, envelope/effective-time behavior, short- and long-term mapping, and + JSON wire shape without `aviation`, `discussion`, or generic `sections` + fields. +5. Keep small parser and scanner edge cases table-driven. Do not multiply full + fixtures for cases that a short line slice proves more clearly. Run and pass: ```sh gofmt -w internal/providers/nws/forecast_discussion_test.go internal/normalizers/nws/forecast_discussion_test.go -go test ./internal/providers/nws ./internal/normalizers/nws +go test -count=1 ./internal/providers/nws ./internal/normalizers/nws ``` -Do not introduce a second full-bulletin fixture unless the checked-in fixture -cannot express a required interaction through guarded heading replacement. +## Stage 8: Reconcile Documentation and Perform Final Validation -## Stage 3: Validate the Feature and Close the Roadmap +Close the resilience follow-up only after all behavior is proven. -Verify the complete change against repository policy and record completion only -after all behavior is proven. - -1. Review the final diff and confirm production changes are confined to the NWS - provider parser. Test changes should be confined to the owning provider and - NWS normalizer packages. Do not retain incidental model, schema, config, - source, sink, Postgres, or current-behavior documentation edits introduced - during implementation, and preserve all pre-existing user work. -2. Run formatting on every changed Go file, then run uncached focused tests and - the full repository suite: +1. Review the final diff. Production changes must remain confined to the NWS + provider parser; other Go changes should be tests in the owning provider and + normalizer packages. Preserve all unrelated user work. +2. Confirm the public contract is unchanged. Do not edit canonical model, + schema, Postgres, config, consumer, or integration documentation unless an + actual contract change is discovered. If one appears necessary, stop rather + than expanding this plan. +3. Run: ```sh gofmt -w \ @@ -184,24 +267,18 @@ after all behavior is proven. internal/normalizers/nws/forecast_discussion_test.go go test -count=1 ./internal/providers/nws ./internal/normalizers/nws go test -count=1 ./... + go vet ./... git diff --check ``` -3. Confirm each acceptance criterion in - [`afd-section-heading-variants.md`](afd-section-heading-variants.md) is - represented by a passing automated test. In particular, verify legacy - compatibility, mixed-format input, slash-qualified aviation termination, - malformed-line non-recognition, normalized qualifiers, and unchanged - canonical wire shape. -4. Because this feature does not alter a public contract, do not change - `docs/integrations/events.md`, `docs/integrations/postgres.md`, consumer docs, - configuration docs, or examples. If implementation appears to require such - a change, stop and reassess the implementation against the feature roadmap - instead of expanding scope. -5. After all checks pass, change the feature roadmap status from `Proposed and - unimplemented.` to `Implemented.` Do not mark it implemented earlier. +4. Verify every acceptance criterion in the feature roadmap has a corresponding + passing automated test, including compatibility with all Stage 1-2 cases. +5. After all checks pass, change the feature roadmap status to `Implemented.` + and rewrite any remaining future-tense statements that would misdescribe the + completed parser. Do not mark the follow-up implemented earlier. ## Open Questions -None. The feature roadmap and this plan fix the accepted grammar, normalization -rules, architectural boundary, test coverage, and canonical compatibility -requirements. +None. This plan fixes the parser grammar, boundary policy, canonical role +selection, multiline preamble rules, presentation cleanup, fixture strategy, +and compatibility boundary. Generic or single-section discussion content remains +outside the canonical model by explicit policy. diff --git a/internal/providers/nws/forecast_discussion.go b/internal/providers/nws/forecast_discussion.go index 60b8cd8..c36aa2a 100644 --- a/internal/providers/nws/forecast_discussion.go +++ b/internal/providers/nws/forecast_discussion.go @@ -27,13 +27,12 @@ type ForecastDiscussionSection struct { Text string } -const ( - forecastDiscussionSectionKeyMessages = "KEY MESSAGES" - forecastDiscussionSectionShortTerm = "SHORT TERM" - forecastDiscussionSectionLongTerm = "LONG TERM" - forecastDiscussionSectionAviation = "AVIATION" +type forecastDiscussionSectionRole uint8 - forecastDiscussionSectionIdentityPattern = "KEY MESSAGES|SHORT TERM|LONG TERM|AVIATION" +const ( + forecastDiscussionSectionRoleKeyMessages forecastDiscussionSectionRole = iota + forecastDiscussionSectionRoleShortTerm + forecastDiscussionSectionRoleLongTerm ) type forecastDiscussionSectionHeading struct { @@ -47,11 +46,14 @@ type forecastDiscussionSectionBlock struct { } var ( - forecastDiscussionEllipsisHeadingRE = regexp.MustCompile(`^\.(` + forecastDiscussionSectionIdentityPattern + `)\.\.\.(.*)$`) - forecastDiscussionSlashHeadingRE = regexp.MustCompile(`^\.(` + forecastDiscussionSectionIdentityPattern + `)[ \t]+/([^/]+)/\.\.\.$`) - forecastDiscussionAFDRE = regexp.MustCompile(`^AFD([A-Z]{3})$`) - forecastDiscussionWMORE = regexp.MustCompile(`\bK([A-Z]{3})\b`) - forecastDiscussionSigRE = regexp.MustCompile(`^[A-Z]{2,6}$`) + forecastDiscussionSectionRoles = map[string]forecastDiscussionSectionRole{ + "KEY MESSAGES": forecastDiscussionSectionRoleKeyMessages, + "SHORT TERM": forecastDiscussionSectionRoleShortTerm, + "LONG TERM": forecastDiscussionSectionRoleLongTerm, + } + forecastDiscussionAFDRE = regexp.MustCompile(`^AFD([A-Z]{3})$`) + forecastDiscussionWMORE = regexp.MustCompile(`\bK([A-Z]{3})\b`) + forecastDiscussionSigRE = regexp.MustCompile(`^[A-Z]{2,6}$`) ) func ParseForecastDiscussionHTML(raw string) (ForecastDiscussion, error) { @@ -119,20 +121,20 @@ func ParseForecastDiscussionText(text string) (ForecastDiscussion, error) { IssuedAt: issuedAt.UTC(), } - if block, ok := extractForecastDiscussionSection(lines, forecastDiscussionSectionKeyMessages); ok { + if block, ok := extractForecastDiscussionSection(lines, forecastDiscussionSectionForRole(forecastDiscussionSectionRoleKeyMessages)); ok { out.KeyMessages = parseForecastDiscussionKeyMessages(block.body) } - if block, ok := extractForecastDiscussionSection(lines, forecastDiscussionSectionShortTerm); ok { + if block, ok := extractForecastDiscussionSection(lines, forecastDiscussionSectionForRole(forecastDiscussionSectionRoleShortTerm)); ok { section, err := parseForecastDiscussionTextSection(block) if err != nil { - return ForecastDiscussion{}, fmt.Errorf("parse %s: %w", forecastDiscussionSectionShortTerm, err) + return ForecastDiscussion{}, fmt.Errorf("parse %s: %w", block.heading.section, err) } out.ShortTerm = §ion } - if block, ok := extractForecastDiscussionSection(lines, forecastDiscussionSectionLongTerm); ok { + if block, ok := extractForecastDiscussionSection(lines, forecastDiscussionSectionForRole(forecastDiscussionSectionRoleLongTerm)); ok { section, err := parseForecastDiscussionTextSection(block) if err != nil { - return ForecastDiscussion{}, fmt.Errorf("parse %s: %w", forecastDiscussionSectionLongTerm, err) + return ForecastDiscussion{}, fmt.Errorf("parse %s: %w", block.heading.section, err) } out.LongTerm = §ion } @@ -408,23 +410,103 @@ func forecastDiscussionLocation(abbrev string) (*time.Location, error) { func parseForecastDiscussionSectionHeading(line string) (forecastDiscussionSectionHeading, bool) { line = strings.TrimSpace(line) - if m := forecastDiscussionEllipsisHeadingRE.FindStringSubmatch(line); len(m) == 3 { - return forecastDiscussionSectionHeading{ - section: m[1], - qualifier: strings.TrimSpace(m[2]), - }, true + if len(line) < 2 || line[0] != '.' { + return forecastDiscussionSectionHeading{}, false } - if m := forecastDiscussionSlashHeadingRE.FindStringSubmatch(line); len(m) == 3 { - qualifier := strings.TrimSpace(m[2]) - if qualifier == "" { - return forecastDiscussionSectionHeading{}, false + + if strings.HasSuffix(line, "/...") { + return parseForecastDiscussionSlashQualifiedHeading(line) + } + return parseForecastDiscussionEllipsisHeading(line) +} + +func parseForecastDiscussionSlashQualifiedHeading(line string) (forecastDiscussionSectionHeading, bool) { + content := strings.TrimSuffix(line[1:], "/...") + separator := -1 + for i := 1; i < len(content); i++ { + if content[i] == '/' && isForecastDiscussionHorizontalWhitespace(content[i-1]) { + separator = i + break } - return forecastDiscussionSectionHeading{ - section: m[1], - qualifier: qualifier, - }, true } - return forecastDiscussionSectionHeading{}, false + if separator < 0 { + return forecastDiscussionSectionHeading{}, false + } + + section, ok := normalizeForecastDiscussionSectionIdentity(content[:separator]) + if !ok { + return forecastDiscussionSectionHeading{}, false + } + qualifier := strings.TrimSpace(content[separator+1:]) + if qualifier == "" { + return forecastDiscussionSectionHeading{}, false + } + return forecastDiscussionSectionHeading{section: section, qualifier: qualifier}, true +} + +func parseForecastDiscussionEllipsisHeading(line string) (forecastDiscussionSectionHeading, bool) { + content := line[1:] + delimiter := strings.Index(content, "...") + if delimiter < 0 { + return forecastDiscussionSectionHeading{}, false + } + + section, ok := normalizeForecastDiscussionSectionIdentity(content[:delimiter]) + if !ok { + return forecastDiscussionSectionHeading{}, false + } + return forecastDiscussionSectionHeading{ + section: section, + qualifier: strings.TrimSpace(content[delimiter+3:]), + }, true +} + +func normalizeForecastDiscussionSectionIdentity(raw string) (string, bool) { + var normalized strings.Builder + pendingSpace := false + hasLetterOrDigit := false + + for i := 0; i < len(raw); i++ { + b := raw[i] + switch { + case isForecastDiscussionIdentityLetterOrDigit(b): + hasLetterOrDigit = true + case b == ' ' || b == '\t': + pendingSpace = normalized.Len() > 0 + continue + case b == '/' && i > 0 && isForecastDiscussionHorizontalWhitespace(raw[i-1]): + return "", false + case b != '/' && b != '&' && b != '\'' && b != '-': + return "", false + } + + if pendingSpace { + normalized.WriteByte(' ') + pendingSpace = false + } + normalized.WriteByte(b) + } + if !hasLetterOrDigit { + return "", false + } + return normalized.String(), true +} + +func isForecastDiscussionIdentityLetterOrDigit(b byte) bool { + return b >= 'A' && b <= 'Z' || b >= '0' && b <= '9' +} + +func isForecastDiscussionHorizontalWhitespace(b byte) bool { + return b == ' ' || b == '\t' +} + +func forecastDiscussionSectionForRole(role forecastDiscussionSectionRole) string { + for section, registeredRole := range forecastDiscussionSectionRoles { + if registeredRole == role { + return section + } + } + return "" } func extractForecastDiscussionSection(lines []string, section string) (forecastDiscussionSectionBlock, bool) { diff --git a/internal/providers/nws/forecast_discussion_test.go b/internal/providers/nws/forecast_discussion_test.go index 50f2f16..8c1f292 100644 --- a/internal/providers/nws/forecast_discussion_test.go +++ b/internal/providers/nws/forecast_discussion_test.go @@ -20,88 +20,150 @@ func TestParseForecastDiscussionSectionHeading(t *testing.T) { { name: "ellipsis-first key messages", line: ".KEY MESSAGES... (Through Tonight)", - wantSection: forecastDiscussionSectionKeyMessages, + wantSection: "KEY MESSAGES", wantQualifier: "(Through Tonight)", wantOK: true, }, { name: "ellipsis-first short term", line: ".SHORT TERM... (Sunday)", - wantSection: forecastDiscussionSectionShortTerm, + wantSection: "SHORT TERM", wantQualifier: "(Sunday)", wantOK: true, }, { name: "ellipsis-first long term", line: ".LONG TERM... (Monday Through Friday)", - wantSection: forecastDiscussionSectionLongTerm, + wantSection: "LONG TERM", wantQualifier: "(Monday Through Friday)", wantOK: true, }, { name: "ellipsis-first aviation without qualifier", line: ".AVIATION...", - wantSection: forecastDiscussionSectionAviation, + wantSection: "AVIATION", wantOK: true, }, { name: "slash-qualified key messages", line: ".KEY MESSAGES /Tonight/...", - wantSection: forecastDiscussionSectionKeyMessages, + wantSection: "KEY MESSAGES", wantQualifier: "Tonight", wantOK: true, }, { name: "slash-qualified short term with outer whitespace", line: " .SHORT TERM /Through Late Sunday Night/... ", - wantSection: forecastDiscussionSectionShortTerm, + wantSection: "SHORT TERM", wantQualifier: "Through Late Sunday Night", wantOK: true, }, { name: "slash-qualified long term", line: ".LONG TERM /Monday through Next Saturday/...", - wantSection: forecastDiscussionSectionLongTerm, + wantSection: "LONG TERM", wantQualifier: "Monday through Next Saturday", wantOK: true, }, { name: "slash-qualified aviation", line: ".AVIATION /18Z TAFS/...", - wantSection: forecastDiscussionSectionAviation, + wantSection: "AVIATION", wantQualifier: "18Z TAFS", wantOK: true, }, { - name: "unknown identity", - line: ".SYNOPSIS... Overview", + name: "generic synopsis", + line: ".SYNOPSIS... Overview", + wantSection: "SYNOPSIS", + wantQualifier: "Overview", + wantOK: true, + }, + { + name: "generic update", + line: ".UPDATE...", + wantSection: "UPDATE", + wantOK: true, + }, + { + name: "generic marine", + line: ".MARINE...", + wantSection: "MARINE", + wantOK: true, + }, + { + name: "generic hydrology", + line: ".HYDROLOGY...", + wantSection: "HYDROLOGY", + wantOK: true, + }, + { + name: "office watch advisory heading", + line: ".LSX WATCHES/WARNINGS/ADVISORIES...", + wantSection: "LSX WATCHES/WARNINGS/ADVISORIES", + wantOK: true, + }, + { + name: "preliminary point temperatures heading", + line: ".PRELIMINARY POINT TEMPS/POPS ...", + wantSection: "PRELIMINARY POINT TEMPS/POPS", + wantOK: true, + }, + { + name: "generic identity punctuation and digits", + line: ".DAY 1 FIRE-WEATHER & HYDROLOGY'S/OUTLOOK...", + wantSection: "DAY 1 FIRE-WEATHER & HYDROLOGY'S/OUTLOOK", + wantOK: true, + }, + { + name: "slash qualifier contains slash", + line: ".LONG TERM /Tonight/Sunday/...", + wantSection: "LONG TERM", + wantQualifier: "Tonight/Sunday", + wantOK: true, + }, + { + name: "normalized identity whitespace", + line: " .SHORT\t\tTERM \t... (Tonight) ", + wantSection: "SHORT TERM", + wantQualifier: "(Tonight)", + wantOK: true, + }, + { + name: "lowercase prose", + line: ".This is ordinary prose...", wantOK: false, }, { - name: "slash qualifier missing opening slash", + name: "empty identity", + line: "....", + wantOK: false, + }, + { + name: "punctuation only identity", + line: ".-/&'...", + wantOK: false, + }, + { + name: "slash qualifier missing opening separator", line: ".SHORT TERM Through Tonight/...", wantOK: false, }, - { - name: "slash qualifier missing closing slash", - line: ".SHORT TERM /Through Tonight...", - wantOK: false, - }, - { - name: "slash qualifier contains embedded slash", - line: ".SHORT TERM /Tonight/Sunday/...", - wantOK: false, - }, - { - name: "slash qualifier whitespace only", - line: ".SHORT TERM / /...", - wantOK: false, - }, { name: "slash qualifier missing required whitespace", line: ".SHORT TERM/Through Tonight/...", wantOK: false, }, + { + name: "slash qualifier missing closing terminal", + line: ".SHORT TERM /Through Tonight...", + wantOK: false, + }, + { + name: "slash qualifier whitespace only", + line: ".SHORT TERM / /...", + wantOK: false, + }, { name: "slash qualifier missing final ellipsis", line: ".SHORT TERM /Through Tonight/..", @@ -113,8 +175,23 @@ func TestParseForecastDiscussionSectionHeading(t *testing.T) { wantOK: false, }, { - name: "lowercase identity", - line: ".short term... (Tonight)", + name: "missing ellipsis", + line: ".SHORT TERM", + wantOK: false, + }, + { + name: "unsupported identity punctuation", + line: ".SHORT TERM:...", + wantOK: false, + }, + { + name: "unicode ellipsis", + line: ".SHORT TERM…", + wantOK: false, + }, + { + name: "mixed case identity", + line: ".Short Term... (Tonight)", wantOK: false, }, } @@ -138,13 +215,34 @@ func TestParseForecastDiscussionSectionHeading(t *testing.T) { } } +func TestForecastDiscussionSectionRoleHeadingsParse(t *testing.T) { + if len(forecastDiscussionSectionRoles) != 3 { + t.Fatalf("role registry has %d entries, want 3", len(forecastDiscussionSectionRoles)) + } + if _, ok := forecastDiscussionSectionRoles["AVIATION"]; ok { + t.Fatalf("AVIATION must remain boundary-only") + } + + for section := range forecastDiscussionSectionRoles { + t.Run(section, func(t *testing.T) { + got, ok := parseForecastDiscussionSectionHeading("." + section + "...") + if !ok { + t.Fatalf("parseForecastDiscussionSectionHeading() did not recognize %q", section) + } + if got.section != section { + t.Fatalf("section = %q, want %q", got.section, section) + } + }) + } +} + func TestExtractForecastDiscussionSectionStopsAtSlashQualifiedHeading(t *testing.T) { block, ok := extractForecastDiscussionSection([]string{ ".LONG TERM /Monday through Next Saturday/...", "Long-term prose.", ".AVIATION /18Z TAFS/...", "Aviation prose.", - }, forecastDiscussionSectionLongTerm) + }, forecastDiscussionSectionForRole(forecastDiscussionSectionRoleLongTerm)) if !ok { t.Fatalf("extractForecastDiscussionSection() found no LONG TERM block") } @@ -162,7 +260,7 @@ func TestExtractForecastDiscussionSectionAllowsEmptyBodyBeforeHeading(t *testing ".SHORT TERM... (Tonight)", ".LONG TERM... (Tomorrow)", "Long-term prose.", - }, forecastDiscussionSectionShortTerm) + }, forecastDiscussionSectionForRole(forecastDiscussionSectionRoleShortTerm)) if !ok { t.Fatalf("extractForecastDiscussionSection() found no SHORT TERM block") } @@ -171,23 +269,23 @@ func TestExtractForecastDiscussionSectionAllowsEmptyBodyBeforeHeading(t *testing } } -func TestExtractForecastDiscussionSectionKeepsUnrecognizedHeadingLikeLines(t *testing.T) { +func TestExtractForecastDiscussionSectionKeepsMalformedHeadingLikeLines(t *testing.T) { block, ok := extractForecastDiscussionSection([]string{ ".SHORT TERM... (Tonight)", - ".SYNOPSIS... This unknown section stays in the body.", + ".short term... Lowercase prose stays in the body.", ".LONG TERM /Tomorrow/..", - ".LONG TERM /Tomorrow/Next Week/...", + ".LONG TERM /Tomorrow/... more", "Expected short-term prose.", ".LONG TERM... (Tomorrow)", "Long-term prose.", - }, forecastDiscussionSectionShortTerm) + }, forecastDiscussionSectionForRole(forecastDiscussionSectionRoleShortTerm)) if !ok { t.Fatalf("extractForecastDiscussionSection() found no SHORT TERM block") } wantBody := []string{ - ".SYNOPSIS... This unknown section stays in the body.", + ".short term... Lowercase prose stays in the body.", ".LONG TERM /Tomorrow/..", - ".LONG TERM /Tomorrow/Next Week/...", + ".LONG TERM /Tomorrow/... more", "Expected short-term prose.", } if !reflect.DeepEqual(block.body, wantBody) {