Complete the Stage 23 refactoring audit
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
# Repository Audit Ledger
|
# Repository Audit Ledger
|
||||||
|
|
||||||
Status: In progress; Stages 1-21 complete.
|
Status: In progress; Stages 1-23 complete.
|
||||||
|
|
||||||
This temporary roadmap document is the evidence ledger for the staged audit
|
This temporary roadmap document is the evidence ledger for the staged audit
|
||||||
defined by [the audit plan](audit-plan.md). It records audit evidence and
|
defined by [the audit plan](audit-plan.md). It records audit evidence and
|
||||||
@@ -177,6 +177,25 @@ semantics; dense configuration/domain tables, realistic filesystem tests,
|
|||||||
small external-boundary fakes, and representative app/CLI workflows earn their
|
small external-boundary fakes, and representative app/CLI workflows earn their
|
||||||
cost. No complete-output golden snapshots or oversized fixture framework was
|
cost. No complete-output golden snapshots or oversized fixture framework was
|
||||||
found.
|
found.
|
||||||
|
Stage 22 found one medium-severity efficiency defect because every Weather API
|
||||||
|
collection discards a current-conditions warmup response, fetches that source
|
||||||
|
again, and serializes all eight otherwise independent source requests. It also
|
||||||
|
found one low-severity replacement cost because an existing comparison bundle
|
||||||
|
is fully recognized four times, causing `4N+16` complete reads for `N`
|
||||||
|
successful reports. Service-sized derivation, fixed registries, template
|
||||||
|
parsing, fixed batch sequencing, and ordinary profile fan-out are otherwise
|
||||||
|
proportionate to their current workloads.
|
||||||
|
Stage 23 found seven low-severity structural issues: daypart-key normalization and
|
||||||
|
Distributor template parsing each have two policy implementations; report
|
||||||
|
metadata and render inputs retain parallel authorities and unused template
|
||||||
|
surfaces; fact requirements span string constants, registry declarations, and
|
||||||
|
availability switches; generate and comparison repeat report-date policy at
|
||||||
|
four sites; several internal compatibility surfaces have no production
|
||||||
|
consumer; and one duplicate capitalization path corrupts non-ASCII daypart
|
||||||
|
labels. Report-specific generated types/builders, explicit prompt allowlist
|
||||||
|
projections, package-local safety copies and path diagnostics, config-to-
|
||||||
|
briefing validation, top-level workflow orchestration, and the current narrow
|
||||||
|
resolved-value clone remain intentionally explicit.
|
||||||
Subsystem conclusions and final disposition remain pending the later stages.
|
Subsystem conclusions and final disposition remain pending the later stages.
|
||||||
|
|
||||||
## Baseline Metadata
|
## Baseline Metadata
|
||||||
@@ -326,7 +345,7 @@ inventory commands, graph index refresh, and graph architecture inspection.
|
|||||||
| 20 | Audit test risk coverage and ownership | Complete |
|
| 20 | Audit test risk coverage and ownership | Complete |
|
||||||
| 21 | Audit test durability, duplication, and maintenance cost | Complete |
|
| 21 | Audit test durability, duplication, and maintenance cost | Complete |
|
||||||
| 22 | Audit cross-cutting efficiency and complexity | Complete |
|
| 22 | Audit cross-cutting efficiency and complexity | Complete |
|
||||||
| 23 | Audit cross-cutting refactoring and deduplication opportunities | Pending |
|
| 23 | Audit cross-cutting refactoring and deduplication opportunities | Complete |
|
||||||
| 24 | Audit documentation coherence and executable contracts | Pending |
|
| 24 | Audit documentation coherence and executable contracts | Pending |
|
||||||
| 25 | Run dynamic robustness and final diagnostic validation | Pending |
|
| 25 | Run dynamic robustness and final diagnostic validation | Pending |
|
||||||
| 26 | Verify, consolidate, and triage findings | Pending |
|
| 26 | Verify, consolidate, and triage findings | Pending |
|
||||||
@@ -360,7 +379,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
|
|||||||
| Test risk coverage and ownership | Repository-wide suite | 20 | Partly insufficient. Consequential contracts have narrow owners and representative cross-boundary workflows; every identified missing regression is already attached to the production or test-gap finding for the defect it would catch. Coverage diagnostics found no additional unowned critical branch. Accepted omissions are thin entry-point/delegation wrappers, defensive accessors, and dormant helpers already routed under `AUD-001`; broad duplication and durability remain Stage 21. |
|
| Test risk coverage and ownership | Repository-wide suite | 20 | Partly insufficient. Consequential contracts have narrow owners and representative cross-boundary workflows; every identified missing regression is already attached to the production or test-gap finding for the defect it would catch. Coverage diagnostics found no additional unowned critical branch. Accepted omissions are thin entry-point/delegation wrappers, defensive accessors, and dormant helpers already routed under `AUD-001`; broad duplication and durability remain Stage 21. |
|
||||||
| Test durability, duplication, and maintenance cost | Repository-wide suite | 21 | Partly insufficient. Most tests assert stable behavior with direct fixtures and appropriately narrow fakes. `AUD-039`, `AUD-061`, and `AUD-063` identify duplicated or implementation-coupled assertions; `AUD-062` identifies avoidable production waits; `AUD-064` identifies incomplete failure cleanup. Each consolidation retains a named behavioral owner. |
|
| Test durability, duplication, and maintenance cost | Repository-wide suite | 21 | Partly insufficient. Most tests assert stable behavior with direct fixtures and appropriately narrow fakes. `AUD-039`, `AUD-061`, and `AUD-063` identify duplicated or implementation-coupled assertions; `AUD-062` identifies avoidable production waits; `AUD-064` identifies incomplete failure cleanup. Each consolidation retains a named behavioral owner. |
|
||||||
| Cross-cutting efficiency | Graph metrics, workflow traces, and focused latency/allocation diagnostics | 22 | Partly insufficient. Fixed-size registries, service-sized derivation, template parsing, batch composition, and ordinary profile fan-out are proportionate. `AUD-065` records one redundant Weather API request plus serial independent source fetches; `AUD-066` records repeated full comparison-bundle reads during replacement. |
|
| Cross-cutting efficiency | Graph metrics, workflow traces, and focused latency/allocation diagnostics | 22 | Partly insufficient. Fixed-size registries, service-sized derivation, template parsing, batch composition, and ordinary profile fan-out are proportionate. `AUD-065` records one redundant Weather API request plus serial independent source fetches; `AUD-066` records repeated full comparison-bundle reads during replacement. |
|
||||||
| Cross-cutting maintainability | Graph metrics plus focused structural review | 23 | Pending |
|
| Cross-cutting maintainability | Graph similarity, change history, caller traces, and focused structural review | 23 | Partly insufficient. `AUD-067` through `AUD-073` identify duplicated canonicalization/template/date/requirement policy, parallel metadata authority, dormant internal surfaces, and unsafe duplicate capitalization. Important report, prompt-curation, safety-copy, validation, and orchestration similarities remain explicit under `RET-053` through `RET-058`. |
|
||||||
| Documentation and executable-contract coherence | Canonical documents, code, schemas, templates, examples | 24 | Pending |
|
| Documentation and executable-contract coherence | Canonical documents, code, schemas, templates, examples | 24 | Pending |
|
||||||
| Dynamic robustness and diagnostic checks | Repository-wide deterministic checks | 25 | Pending |
|
| Dynamic robustness and diagnostic checks | Repository-wide deterministic checks | 25 | Pending |
|
||||||
|
|
||||||
@@ -2897,6 +2916,298 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
|
|||||||
- Related findings: `AUD-038`, `AUD-053`, `AUD-055`
|
- Related findings: `AUD-038`, `AUD-053`, `AUD-055`
|
||||||
- Remediation reference: pending
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-067: Daypart key canonicalization has two independent owners
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: duplication
|
||||||
|
- Area: `internal/briefing.normalizedKey` and
|
||||||
|
`internal/generatedtext.normalizedDaypartKey`
|
||||||
|
- Evidence: Graph fingerprint comparison found two exact 17-line production
|
||||||
|
implementations: trim and lowercase the name, preserve Unicode letters and
|
||||||
|
digits, collapse every other run to one underscore, and trim edge
|
||||||
|
underscores. Briefing introduced its copy in `7b760a0`; generated-text added
|
||||||
|
a separately named copy later in `ddda424`. Briefing uses the key to store
|
||||||
|
daypart module values, while generated-text recomputes it from derived facts
|
||||||
|
to recover configured order. The same identity is also the collision domain
|
||||||
|
whose missing validation causes `AUD-024`.
|
||||||
|
- Contract at risk: One accepted daypart name must have one canonical identity
|
||||||
|
from configuration through module storage and render ordering.
|
||||||
|
- Impact: A future Unicode, punctuation, casing, or collision-policy change
|
||||||
|
must update two packages identically. Drift can leave a module entry present
|
||||||
|
under one key but unrecognized by ordered rendering under the other, causing
|
||||||
|
misplaced or omitted daypart output even after collision validation is
|
||||||
|
added.
|
||||||
|
- Recommendation: Give canonical daypart identity to the existing daypart
|
||||||
|
domain owner, preferably beside `forecast.DaypartDefinition`, and make
|
||||||
|
configuration collision checks, briefing keys, and generated-text ordering
|
||||||
|
call that one helper. Keep display labels separate from canonical roles as
|
||||||
|
required by `AUD-025`; do not introduce a generic string-normalization
|
||||||
|
package.
|
||||||
|
- Test implications: One focused domain table should own blank edges,
|
||||||
|
punctuation runs, casing, Unicode, and collision equivalence. Config should
|
||||||
|
test rejection consequences, while briefing/generated-text retain one
|
||||||
|
composition case each rather than repeating the normalization table.
|
||||||
|
- Validation: Every accepted name produces the same key at validation,
|
||||||
|
snapshot, prompt, and render boundaries; current default keys and rendered
|
||||||
|
ordering remain unchanged.
|
||||||
|
- Related findings: `AUD-024`, `AUD-025`
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-068: Distributor single and batch templates duplicate one parser
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: duplication
|
||||||
|
- Area: `internal/config.renderDistributorTemplate` and
|
||||||
|
`renderDistributorBatchTemplate`
|
||||||
|
- Evidence: The two 27-line functions are fingerprint-identical except for
|
||||||
|
their value type and variable-to-value callback. Both independently own the
|
||||||
|
brace scanner and the exact unclosed, empty, unknown, and unopened-variable
|
||||||
|
diagnostics. Batch support copied the second grammar in `32060bd`; the file
|
||||||
|
has seven historical changes, and config tests repeat malformed/unknown
|
||||||
|
template cases for the two families. Typed public wrappers and allowed
|
||||||
|
variable sets are legitimately distinct, but syntax is not.
|
||||||
|
- Contract at risk: Single-report and batch notification templates use one
|
||||||
|
interpolation grammar and must reject malformed syntax consistently.
|
||||||
|
- Impact: Any escaping, validation, diagnostic, or parser correction requires
|
||||||
|
synchronized edits and duplicated tests. Drift would make configuration
|
||||||
|
accept a template for one notification mode and reject the same syntax for
|
||||||
|
the other without a domain reason.
|
||||||
|
- Recommendation: Extract one private parser that accepts the diagnostic name
|
||||||
|
and a typed-family resolver callback returning a value and whether the
|
||||||
|
variable is allowed. Retain distinct value structs, allowed-variable sets,
|
||||||
|
nonempty-result rules, and public rendering functions; no general template
|
||||||
|
framework or new package is warranted.
|
||||||
|
- Test implications: Give the shared parser one malformed-syntax table. Keep
|
||||||
|
small single/batch tests for their variable vocabularies, value mappings,
|
||||||
|
nonempty requirements, report-path validation, and public error context.
|
||||||
|
- Validation: Every current valid template renders identically, current
|
||||||
|
malformed inputs retain their actionable field names, and a parser mutation
|
||||||
|
is detected once rather than by copied syntax suites.
|
||||||
|
- Related findings: none
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-069: Preparation retains parallel metadata authorities and unused render surfaces
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: architecture
|
||||||
|
- Area: `internal/briefing.BuildMetadata`, `buildMetadataModule`,
|
||||||
|
`internal/app.preparedReport`, and
|
||||||
|
`internal/generatedtext/*RenderContext`/`*TemplateModules`
|
||||||
|
- Evidence: `BuildMetadata` and `buildMetadataModule` independently project the
|
||||||
|
same run ID, report ID, variant, prompt ID, generation time, units, timezone,
|
||||||
|
valid period, location, and source-warning facts. Preparation retains the
|
||||||
|
broader `briefing.Metadata` beside a snapshot containing `MetadataModule`,
|
||||||
|
then projects the former again into `promptinput.Metadata`. Render contexts
|
||||||
|
receive the outer metadata for display identity while also exposing the
|
||||||
|
decoded module metadata. All four contexts additionally expose complete
|
||||||
|
`Collected` and `Derived` values. Repository template search found no use of
|
||||||
|
`.Collected`, `.Derived`, or `.Modules.Metadata`; the duplicated identity
|
||||||
|
paths already permit the conflicts in `AUD-040`. The broad metadata path
|
||||||
|
predates the later module and render-context paths, rather than resulting
|
||||||
|
from one shared projection.
|
||||||
|
- Contract at risk: Prompt metadata, metadata stanza content, and displayed
|
||||||
|
report timing must derive from one prepared identity, while templates should
|
||||||
|
receive only fields they are authorized to render.
|
||||||
|
- Impact: A metadata addition or correction can require parallel builders,
|
||||||
|
application projection, snapshot fixtures, and context fixtures. More
|
||||||
|
importantly, callers can supply conflicting outer and stanza identities and
|
||||||
|
templates gain accidental access to raw deterministic facts that current
|
||||||
|
assets deliberately do not use.
|
||||||
|
- Recommendation: After resolving `AUD-040`, establish one smallest prepared
|
||||||
|
metadata value and derive prompt-package metadata, the metadata stanza, and
|
||||||
|
report display context from it through named projections. Remove unused
|
||||||
|
`Collected`/`Derived` fields and the unused template-module metadata field;
|
||||||
|
pass derived facts privately only while constructing ordered daypart views.
|
||||||
|
Keep prompt-package ownership in `promptinput` and module-value ownership in
|
||||||
|
`briefing` rather than replacing them with one cross-layer mega-struct.
|
||||||
|
- Test implications: Preserve one preparation test proving all three outputs
|
||||||
|
share identity and source-warning values, module tests for prompt-safe stanza
|
||||||
|
content, generated-text tests for report display fields and daypart order,
|
||||||
|
and template tests for unchanged Markdown. Delete fixture data that protects
|
||||||
|
only unreachable template fields.
|
||||||
|
- Validation: Current prompt YAML and rendered Markdown remain semantically or
|
||||||
|
byte equivalent as contractually required, conflicting identity inputs are
|
||||||
|
impossible or rejected, and template contexts contain no unused complete
|
||||||
|
fact bundle.
|
||||||
|
- Related findings: `AUD-029`, `AUD-040`, `AUD-063`
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-070: Fact requirement policy spans constants, declarations, and switches
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: architecture
|
||||||
|
- Area: `internal/module.FactRequirement`,
|
||||||
|
`internal/briefing.defaultModuleDefinitions`, `collectedFactAvailable`, and
|
||||||
|
`derivedFactAvailable`
|
||||||
|
- Evidence: Adding a module fact prerequisite requires a string constant in
|
||||||
|
`module`, use in one or more registry definitions, and a matching branch in
|
||||||
|
one of two availability switches in `briefing`. `FactRequirement` is a
|
||||||
|
string, and `NewModuleRegistry` validates neither known requirement names nor
|
||||||
|
collected-versus-derived placement; an unknown value is accepted and later
|
||||||
|
silently treated as unavailable. `modules.go` has changed 21 times as source,
|
||||||
|
derived, planning, SPC, and prompt-export modules were added, demonstrating
|
||||||
|
continuing evolution of this three-part policy. Focused default-composition
|
||||||
|
tests currently keep built-ins aligned but do not make one representation
|
||||||
|
authoritative.
|
||||||
|
- Contract at risk: Every declared prerequisite must have one known
|
||||||
|
availability predicate, and registry construction should reject rather than
|
||||||
|
defer unknown policy vocabulary.
|
||||||
|
- Impact: A new or renamed requirement can compile and validate into a registry
|
||||||
|
yet make its module always missing at runtime. The separate collected and
|
||||||
|
derived slices also allow a valid constant in the wrong category, producing
|
||||||
|
the same misleading absence without identifying the declaration error.
|
||||||
|
- Recommendation: Move requirement descriptors to `briefing`, where facts are
|
||||||
|
visible, and define each name, fact category, and availability predicate
|
||||||
|
once. Let definitions refer to those descriptors or validate identifiers
|
||||||
|
through one registry-owned map during `NewModuleRegistry`. Keep each
|
||||||
|
module's actual requirement list and missing-data behavior explicit in the
|
||||||
|
definition table.
|
||||||
|
- Test implications: One descriptor table should prove every identifier has a
|
||||||
|
predicate and correct category; constructor tests should reject unknown or
|
||||||
|
misplaced requirements. Retain behavior tests for materially distinct
|
||||||
|
missing-source policies, not one availability test per constant.
|
||||||
|
- Validation: Every default definition constructs, unknown/miscategorized
|
||||||
|
requirements fail at registry creation, and existing omit/empty/error results
|
||||||
|
remain unchanged for all reports.
|
||||||
|
- Related findings: none
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-071: Generate and comparison repeat report-date policy at four sites
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: duplication
|
||||||
|
- Area: `internal/cli.parseGenerateFlags`, `parseComparisonFlags`,
|
||||||
|
`Runner.resolveGenerateAction`, and `Runner.resolveComparisonAction`
|
||||||
|
- Evidence: Both flag parsers independently decide that Daily and Today accept
|
||||||
|
`--date`. Both action resolvers then independently require it for Daily,
|
||||||
|
default it to the local current date for Today, parse it in the configured
|
||||||
|
timezone, and leave Tomorrow/Hourly without it. Generate acquired Today
|
||||||
|
policy in `4a02389`; comparison copied the action policy later in `b489c56`.
|
||||||
|
`root.go` has 36 historical changes, and
|
||||||
|
`TestResolveComparisonActionMatchesReportDatePolicies` exists specifically to
|
||||||
|
detect disagreement after the fact. Action-specific request construction and
|
||||||
|
diagnostics differ, but report/date capability does not.
|
||||||
|
- Contract at risk: The same report accepts, requires, defaults, and parses the
|
||||||
|
same date semantics regardless of whether one profile or several profiles
|
||||||
|
execute it.
|
||||||
|
- Impact: Adding a report variant or changing date semantics requires four
|
||||||
|
coordinated branches plus agreement tests. A missed site can expose a flag
|
||||||
|
that later fails, reject a valid comparison only, or choose different civil
|
||||||
|
dates for otherwise equivalent actions.
|
||||||
|
- Recommendation: Add one private CLI report-date policy descriptor and one
|
||||||
|
resolver that returns the effective local date from report kind, supplied
|
||||||
|
text, timezone, and clock. Let each parser consult `AcceptsDate` and each
|
||||||
|
action wrap missing-date diagnostics with its command name. Keep flag parsing
|
||||||
|
and request construction in CLI rather than moving them into the report
|
||||||
|
registry.
|
||||||
|
- Test implications: Give the shared policy a four-report table for acceptance,
|
||||||
|
required/default/forbidden behavior, invalid dates, and timezone-relative
|
||||||
|
Today defaults. Retain one thin generate and comparison wiring case each.
|
||||||
|
- Validation: Generate and compare preserve current accepted flags, errors, and
|
||||||
|
resolved dates for all four reports, and no duplicated report-kind switch
|
||||||
|
remains in their date path.
|
||||||
|
- Related findings: `AUD-009`
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-072: Dormant internal compatibility surfaces have no workflow owner
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: architecture
|
||||||
|
- Area: `internal/promptinput.LoadYAML`,
|
||||||
|
`internal/forecast.{DifferenceAtLeast,CrossesAtOrAbove,CrossesBelow}`,
|
||||||
|
`internal/weatherdata.AlertRun.Raw`, and the normalized-JSON return from
|
||||||
|
`internal/generatedtext` validators
|
||||||
|
- Evidence: Production caller traces find no caller for `LoadYAML`; its
|
||||||
|
round-trip tests are the only consumer, while normal execution builds a
|
||||||
|
package and marshals YAML directly. The three exported forecast threshold
|
||||||
|
helpers are called only by `TestThresholdHelpers`. The Weather API adapter
|
||||||
|
copies every full alerts response into `AlertRun.Raw`, but no production or
|
||||||
|
test code reads that field; individual raw alert items and source hashes own
|
||||||
|
the needed payload/provenance. All four generated-text validators marshal and
|
||||||
|
return canonical normalized JSON, but the sole production handler caller
|
||||||
|
assigns that result to `_`; normalization behavior is otherwise consumed
|
||||||
|
through the typed value used for rendering. The persistence half of the same
|
||||||
|
obsolete prompt-input surface is already `AUD-001`, and hardening the unused
|
||||||
|
loader alone is `AUD-032`.
|
||||||
|
- Contract at risk: Internal package APIs and stored values should describe the
|
||||||
|
active stateless pipeline, not imply compatibility or persistence contracts
|
||||||
|
with no workflow owner.
|
||||||
|
- Impact: These surfaces retain tests, error branches, allocations, JSON shape,
|
||||||
|
and future design constraints without protecting current output. They can
|
||||||
|
invite a new caller to rely on accidental semantics such as YAML round-trip
|
||||||
|
loading, threshold helpers with no domain owner, a duplicate whole alert
|
||||||
|
envelope, or canonical JSON that the renderer never observes.
|
||||||
|
- Recommendation: Retire `LoadYAML` with `promptinput.Save`; remove the unused
|
||||||
|
threshold helpers and their representation-only test; remove `AlertRun.Raw`
|
||||||
|
and adapter assignments; and simplify the internal validator/handler result
|
||||||
|
to the typed value plus error. If a concrete tooling consumer is identified
|
||||||
|
before remediation, document and test its narrow contract instead of
|
||||||
|
retaining all surfaces speculatively.
|
||||||
|
- Test implications: Preserve Build/Validate/MarshalYAML execution tests,
|
||||||
|
forecast behavior that uses real threshold decisions, alert item parsing and
|
||||||
|
provenance hashes, typed normalization semantics, schema agreement, and all
|
||||||
|
normal generate/batch/comparison workflows. Remove only tests whose sole
|
||||||
|
subject is the retired surface; `AUD-039` still owns duplicated validator
|
||||||
|
semantics.
|
||||||
|
- Validation: Graph and text searches show no remaining caller or field
|
||||||
|
reference, prompt YAML and rendered reports remain unchanged, alert source
|
||||||
|
hashes still cover accepted payloads, and the repository suite passes.
|
||||||
|
- Related findings: `AUD-001`, `AUD-032`, `AUD-038`, `AUD-039`
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
|
### AUD-073: Duplicate capitalization helpers disagree on Unicode safety
|
||||||
|
|
||||||
|
- Stage: 23
|
||||||
|
- Status: candidate
|
||||||
|
- Severity: low
|
||||||
|
- Confidence: high
|
||||||
|
- Category: duplication
|
||||||
|
- Area: `internal/briefing.titleWord` and `sentenceCase`
|
||||||
|
- Evidence: `sentenceCase` trims, converts the string to runes, and uppercases
|
||||||
|
the first rune. `titleWord` instead uppercases `value[:1]` and appends
|
||||||
|
`value[1:]`, splitting any multibyte first rune. Its four production callers
|
||||||
|
place configured daypart names into derived summaries, workday concerns,
|
||||||
|
outdoor planning notes, and late-day watch prose. Configuration accepts
|
||||||
|
Unicode nonblank daypart names, so a name beginning with `é`, `ø`, or another
|
||||||
|
multibyte letter reaches this path and produces replacement/invalid text.
|
||||||
|
Existing tests use ASCII default names only.
|
||||||
|
- Contract at risk: Accepted Unicode display labels must remain valid and
|
||||||
|
readable when interpolated into deterministic briefing prose.
|
||||||
|
- Impact: A valid localized or operator-chosen daypart name can be corrupted in
|
||||||
|
several prompt-facing planning fields. Two helpers that express the same
|
||||||
|
first-character presentation policy also invite future call sites to choose
|
||||||
|
the unsafe implementation accidentally.
|
||||||
|
- Recommendation: Replace both with one private rune-safe first-letter helper,
|
||||||
|
choosing trimming explicitly at each call boundary if their whitespace
|
||||||
|
contracts differ. Keep canonical identity normalization separate under
|
||||||
|
`AUD-067`; this helper is display-only and must not assign semantic roles.
|
||||||
|
- Test implications: Add one small table for empty, ASCII, multibyte lowercase,
|
||||||
|
already-uppercase, and leading-space behavior. Retain one assembled planning
|
||||||
|
assertion proving a Unicode daypart label survives in prompt-facing prose.
|
||||||
|
- Validation: All current ASCII output is unchanged, every accepted Unicode
|
||||||
|
label remains valid UTF-8 with its first rune capitalized, and only one
|
||||||
|
capitalization implementation remains.
|
||||||
|
- Related findings: `AUD-024`, `AUD-025`, `AUD-067`
|
||||||
|
- Remediation reference: pending
|
||||||
|
|
||||||
## Retained Decisions
|
## Retained Decisions
|
||||||
|
|
||||||
### RET-001: Keep the application package as the explicit composition owner
|
### RET-001: Keep the application package as the explicit composition owner
|
||||||
@@ -3515,6 +3826,77 @@ observable clone contention, or a larger dynamic batch as triggers for a
|
|||||||
bounded worker design rather than inferring a defect from goroutine or graph
|
bounded worker design rather than inferring a defect from goroutine or graph
|
||||||
counts alone.
|
counts alone.
|
||||||
|
|
||||||
|
### RET-053: Keep report-specific generated types and thin builders explicit
|
||||||
|
|
||||||
|
Fingerprint analysis found exact or near-exact Daily/Tomorrow context builders,
|
||||||
|
four catalog type-dispatch adapters, three planning-module wrappers, and
|
||||||
|
parallel report definitions. Their similarity follows four intentionally
|
||||||
|
distinct report contracts: each public generated type, schema/template pair,
|
||||||
|
planning stanza, report ID, and typed context must remain independently
|
||||||
|
selectable and type-checked. Existing private day-style helpers already share
|
||||||
|
the mechanics that have one policy owner. Retain the thin variants rather than
|
||||||
|
introducing reflection, a generic report mega-type, or callbacks that obscure
|
||||||
|
which planning fields each report supports. `AUD-039` and `AUD-063` own test
|
||||||
|
duplication without erasing these production distinctions.
|
||||||
|
|
||||||
|
### RET-054: Keep rich-to-prompt projections as explicit allowlists
|
||||||
|
|
||||||
|
Current conditions, hourly periods, and derived dayparts copy many fields from
|
||||||
|
rich module values into narrower prompt structs. That field-by-field code is
|
||||||
|
deliberate curation: omission of template-only helpers, source detail, and
|
||||||
|
unneeded values is part of the prompt security and authority boundary under
|
||||||
|
`RET-017`. Code generation, reflection, embedding, or serialization through
|
||||||
|
the rich struct would make accidental prompt expansion harder to review.
|
||||||
|
Retain named projection functions and focused omission tests. Extract only
|
||||||
|
small semantic subprojections when multiple prompt types genuinely share the
|
||||||
|
same allowlist and change history.
|
||||||
|
|
||||||
|
### RET-055: Keep tiny safety copies and path checks package-local
|
||||||
|
|
||||||
|
Exact fingerprints found three ten-line string-map copy helpers in Promptkit,
|
||||||
|
prompt-debug, and neutral execution packages, plus similar absolute-path
|
||||||
|
checks in app and comparison. The copies sit at separate mutation/security
|
||||||
|
boundaries and their names state which owner is taking a copy. The path checks
|
||||||
|
attach different diagnostics and feed substantially different publication
|
||||||
|
policies. A generic utility package would save little code while coupling
|
||||||
|
unrelated contracts. Retain these local helpers; share only if a common type or
|
||||||
|
policy owner emerges, not because the mechanics happen to match.
|
||||||
|
|
||||||
|
### RET-056: Keep configuration validation against the briefing registry
|
||||||
|
|
||||||
|
`internal/config` imports `internal/briefing` to normalize typed module options
|
||||||
|
and validate report compositions against the same definitions used at runtime.
|
||||||
|
This edge prevents configuration from copying module IDs, option shapes,
|
||||||
|
supported-report rules, and stanza policy. Moving a second descriptor table
|
||||||
|
into config would create the duplication Stage 23 is meant to remove, while a
|
||||||
|
new abstraction package would split the current owner without another
|
||||||
|
consumer. Retain the dependency; narrow the requirement vocabulary within
|
||||||
|
briefing under `AUD-070` without moving runtime facts into config.
|
||||||
|
|
||||||
|
### RET-057: Keep workflow orchestration visibly distinct
|
||||||
|
|
||||||
|
Generate, fixed batch, and comparison share phases such as resolution,
|
||||||
|
inspection, collection, preparation, execution, and publication, but the
|
||||||
|
ordering and failure contracts differ materially. Batch validates all outputs,
|
||||||
|
continues independent report failures, publishes sequentially, and gates one
|
||||||
|
notification. Comparison validates explicit profiles, prepares once, executes
|
||||||
|
concurrently, and publishes one partial-capable transaction without
|
||||||
|
notification. Extracting a common workflow framework would hide those state
|
||||||
|
machines. Retain top-level guarded pipelines and consider only phase-local
|
||||||
|
helpers after `AUD-051`, `AUD-058`, `AUD-065`, and `AUD-066` settle their
|
||||||
|
behavior.
|
||||||
|
|
||||||
|
### RET-058: Keep the resolved-value clone explicit for current option shapes
|
||||||
|
|
||||||
|
`cloneResolved` copies report slices and the only reference-bearing module
|
||||||
|
option, AFD sections. Every other accepted option is presently value-only, and
|
||||||
|
mutation-isolation tests cover the complete prepared boundary. Reflection or
|
||||||
|
JSON cloning would add runtime failure modes and obscure ownership to solve a
|
||||||
|
hypothetical future shape. Retain the explicit switch now. If a second
|
||||||
|
reference-bearing option is introduced, give module options an owner-defined
|
||||||
|
clone contract and make the registry/construction test prove every registered
|
||||||
|
shape participates.
|
||||||
|
|
||||||
## Open Questions
|
## Open Questions
|
||||||
|
|
||||||
No Stage 1 open questions or unexplained baseline failures remain.
|
No Stage 1 open questions or unexplained baseline failures remain.
|
||||||
@@ -3909,6 +4291,25 @@ Stage 22 routed these investigation leads to their assigned later stages:
|
|||||||
Stage 26 may consider a limit or worker bound only if operational evidence
|
Stage 26 may consider a limit or worker bound only if operational evidence
|
||||||
establishes a larger supported workload.
|
establishes a larger supported workload.
|
||||||
|
|
||||||
|
Stage 23 routed these investigation leads to their assigned later stages:
|
||||||
|
|
||||||
|
- Stage 24 should reconcile docs that present `promptinput.Save`/`LoadYAML`,
|
||||||
|
normalized generated JSON, complete render-context facts, or template module
|
||||||
|
metadata as maintained surfaces if Stage 26 selects the retirement/narrowing
|
||||||
|
direction in `AUD-069` and `AUD-072`. Current-state documentation should not
|
||||||
|
be changed before remediation.
|
||||||
|
- Stage 25 should include a Unicode-leading daypart label in its broader
|
||||||
|
parser/configuration corpus and preserve the existing destination-race cases;
|
||||||
|
it should validate the findings rather than implement shared helpers.
|
||||||
|
- Stage 26 should consolidate `AUD-001`, `AUD-032`, and `AUD-072` into one
|
||||||
|
coherent dormant-surface decision; `AUD-024`, `AUD-025`, `AUD-067`, and
|
||||||
|
`AUD-073` into one daypart identity/display sequence; and `AUD-040` with
|
||||||
|
`AUD-069` so canonical metadata ownership precedes context narrowing.
|
||||||
|
- Refactoring for `AUD-065` should keep source-local collection results and one
|
||||||
|
deterministic merge point; refactoring for `AUD-066` should keep initial
|
||||||
|
rejection and commit-time authorization in their current safety owners.
|
||||||
|
Stage 26 must sequence those behavior fixes before extracting helpers.
|
||||||
|
|
||||||
## Stage Log
|
## Stage Log
|
||||||
|
|
||||||
### Stage 1: Establish The Baseline And Audit Ledger
|
### Stage 1: Establish The Baseline And Audit Ledger
|
||||||
@@ -5136,3 +5537,65 @@ Stage 22 routed these investigation leads to their assigned later stages:
|
|||||||
- Retained decisions: `RET-050` through `RET-052`.
|
- Retained decisions: `RET-050` through `RET-052`.
|
||||||
- Open questions: the four leads recorded above are routed to their assigned
|
- Open questions: the four leads recorded above are routed to their assigned
|
||||||
later stages.
|
later stages.
|
||||||
|
|
||||||
|
### Stage 23: Audit Cross-Cutting Refactoring And Deduplication Opportunities
|
||||||
|
|
||||||
|
- Status: Complete.
|
||||||
|
- Scope reviewed: graph exact-fingerprint and semantic-similarity candidates;
|
||||||
|
repeated report/date policies, mappings, validation, path handling, result
|
||||||
|
and error projections, report and module variants, workflow stages, template
|
||||||
|
parsing, canonicalization, value copying, prompt curation, generated-text
|
||||||
|
APIs, render contexts, registry construction, fact requirements, dormant
|
||||||
|
compatibility surfaces, test ownership already established in Stage 21, and
|
||||||
|
production/test-file change history.
|
||||||
|
- Exclusions: no structural remediation was implemented; documentation
|
||||||
|
coherence remains Stage 24; adversarial/dynamic validation remains Stage 25;
|
||||||
|
consolidation, priority, and remediation sequencing remain Stages 26-27. No
|
||||||
|
production code or tests were changed.
|
||||||
|
|
||||||
|
#### Refactoring And Duplication Accounting
|
||||||
|
|
||||||
|
| Candidate | Shared behavior, drift/change evidence, and required stable boundary | Disposition |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| Daypart canonical keys | Briefing and generated-text contain exact 17-line normalizers introduced in different commits. Both must map accepted configuration names to the same snapshot/render key; collisions are already consequential under `AUD-024`. | One domain owner under `AUD-067`; preserve display labels and semantic roles separately. |
|
||||||
|
| Distributor template grammar | Single and batch rendering contain exact 27-line brace parsers and duplicated malformed-input tests. Batch copied the grammar later; typed values and variable sets differ, syntax does not. | One private parser callback under `AUD-068`; retain typed public wrappers and family policies. |
|
||||||
|
| Prepared/report metadata | Two briefing builders repeat ten identity/context fields; app retains outer metadata plus a metadata stanza and projects prompt metadata again. Templates use neither complete facts nor module metadata exposed by all contexts. Existing conflict is `AUD-040`. | Establish one prepared identity and narrow projections under `AUD-069`; preserve prompt YAML and Markdown contracts. |
|
||||||
|
| Fact requirements | Names live in `module`, lists in briefing definitions, and availability in two switches; constructor accepts unknown or miscategorized strings. `modules.go` has 21 revisions across continued module growth. | Define descriptor and predicate once under `AUD-070`; keep per-module lists/missing policy explicit. |
|
||||||
|
| CLI report dates | Generate and comparison each repeat flag eligibility plus required/default/parse logic. Policies arrived in different commits, and a cross-action test now guards agreement in a 36-change file. | One private CLI descriptor/resolver under `AUD-071`; preserve action-specific errors and request types. |
|
||||||
|
| Dormant compatibility surfaces | YAML loading, three forecast threshold exports, complete alert envelopes, and canonical normalized generated JSON have no production consumer; some retain tests or runtime copies. | Retire together with related `AUD-001`/`AUD-032` work under `AUD-072`, unless Stage 26 identifies a concrete owner. |
|
||||||
|
| Capitalization | Rune-safe `sentenceCase` and byte-slicing `titleWord` express the same presentation mechanic; the latter has four prompt-facing callers and corrupts accepted multibyte labels. | One display-only rune-safe helper under `AUD-073`; canonical identity remains `AUD-067`. |
|
||||||
|
| Report variants | Daily/Tomorrow context builders are fingerprint-identical and catalog/planning wrappers are similar, but distinct types, schema/template pairs, IDs, and planning fields are intentional contracts. | Retain thin explicit variants under `RET-053`; existing private day-style helpers own shared mechanics. |
|
||||||
|
| Prompt projections | Rich current/hourly/daypart values require lengthy field copies into narrower prompt types. Omitted fields are an authority allowlist, not incidental mapping repetition. | Retain reviewable projections under `RET-054`; no reflection, embedding, or generated mega-mapper. |
|
||||||
|
| Maps and absolute paths | Three packages locally copy string maps; app/comparison perform similar absolute-clean checks. Copies protect separate mutation boundaries, while path errors and downstream policy differ. | Retain tiny local helpers under `RET-055`; no generic utility package. |
|
||||||
|
| Config and briefing registry | Config calls the runtime briefing registry for module IDs, option types, supported reports, and composition validation. | Retain under `RET-056`; duplicating a config descriptor would increase drift. |
|
||||||
|
| Workflow and result translation | Generate, batch, and comparison share phase names but differ in preflight scope, continuation, concurrency, transaction, notification, partial results, and safe CLI projection. | Retain visible state machines and boundary-specific result/error projections under `RET-057`; extract only local phases after open correctness findings settle. |
|
||||||
|
| Prepared clone | `cloneResolved` manually copies slices and the sole reference-bearing option; current mutation tests cover it. | Retain under `RET-058`; introduce an option-owned clone contract only when another reference-bearing shape appears. |
|
||||||
|
| Test fixtures and duplicate assertions | Stage 21 already identified the concrete cross-owner and implementation-coupled cases in `AUD-039`, `AUD-061`, and `AUD-063`; remaining local fixtures/tables protect distinct contracts. | Do not create a shared fixture framework or duplicate Stage 21 findings. |
|
||||||
|
|
||||||
|
#### Commands And Evidence
|
||||||
|
|
||||||
|
- Enumerated all indexed function nodes and grouped exact fingerprints, then
|
||||||
|
manually separated tests, thin report variants, local safety helpers, and
|
||||||
|
genuine shared policy. Used semantic graph search, symbol snippets, and
|
||||||
|
inbound traces for metadata, requirements, projections, date handling,
|
||||||
|
workflows, and dormant APIs.
|
||||||
|
- Used targeted text search where assets and field references are outside graph
|
||||||
|
coverage. Confirmed no template references `.Collected`, `.Derived`, or
|
||||||
|
`.Modules.Metadata`; no production caller consumes validator normalized JSON,
|
||||||
|
`AlertRun.Raw`, the three forecast thresholds, or `LoadYAML`.
|
||||||
|
- Reviewed history rather than inferring burden from line similarity alone.
|
||||||
|
`root.go` has 36 changes, `briefing/modules.go` 21, and
|
||||||
|
`config/notify_templates.go` seven. The two daypart normalizers and the two
|
||||||
|
CLI action policies were introduced in distinct commits; batch notification
|
||||||
|
copied the existing template grammar when that feature arrived.
|
||||||
|
- Reconciled every earlier Stage 23 lead: unused persistence/loader surfaces
|
||||||
|
join `AUD-072`; metadata and render-context breadth become `AUD-069`;
|
||||||
|
requirements become `AUD-070`; date policy becomes `AUD-071`; Unicode and
|
||||||
|
daypart identity become `AUD-067`/`AUD-073`; projections, config coupling,
|
||||||
|
workflow metrics, and the current clone remain retained decisions.
|
||||||
|
- Ran `go test ./...`, `go vet ./...`,
|
||||||
|
`go run ./cmd/weatherreporter --help`, and `git diff --check`; all passed.
|
||||||
|
- Findings: `AUD-067` through `AUD-073`.
|
||||||
|
- Retained decisions: `RET-053` through `RET-058`.
|
||||||
|
- Open questions: the four leads recorded above are routed to their assigned
|
||||||
|
later stages.
|
||||||
|
|||||||
Reference in New Issue
Block a user