LintMonster group 3: harden type assertions, reduce manifest updater params, and fix file-close defer

docs/adr/35111-harden-runtime-type-assertions-with-per-site-recovery.md

@@ -0,0 +1,91 @@
+# ADR-35111: Harden Runtime Type Assertions With Per-Site Recovery
+
+**Date**: 2026-05-27
+**Status**: Draft
+**Deciders**: Unknown — generated by the Design Decision Gate from PR #35111 evidence
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+The repository has accumulated a number of single-value Go type assertions of the form `v := x.(T)` against values pulled from `sync.Map` caches, `map[string]any` schema fragments, and `analysis.Pass.ResultOf`. Each of these panics on a type mismatch, which would take down a long-lived linter pass, compile, or workflow update operation rather than surface a recoverable error. The repo's custom `uncheckedtypeassertion` linter (see [ADR-34738](34738-add-uncheckedtypeassertion-linter.md)) flagged 31 such sites across `pkg/parser`, `pkg/workflow`, `pkg/cli`, and `pkg/linters/*`. At the same time the `excessivefuncparams` and `fileclosenotdeferred` linters flagged one site each in `pkg/cli/update_manifest.go` and `pkg/cli/workflows.go`. We needed a consistent remediation strategy that converts panics into appropriate, context-specific behavior without losing diagnostic signal.
+
+### Decision
+
+We will **replace every flagged unchecked single-value type assertion with a two-value assertion plus per-call-site recovery**, choosing one of four recovery modes based on what the call site can do safely:
+
+1. **Return an error** when the call site already has an `error` return and a typed cache invariant has been violated (e.g. `repositoryFeaturesCache`).
+2. **Drop the bad cache entry and reconstruct** when the cache is local, recomputable, and self-healing is safe (e.g. `unquoteYAMLKeyCache`, `builtinFrontmatterCache`).
+3. **Append a structured warning and skip the affected output** when the function aggregates warnings and partial output is acceptable (e.g. `convertTriggersToOn`).
+4. **Initialize a fresh map / slice** when a schema-shaped `map[string]any` is malformed but the function's job is to construct it (e.g. `generateActionToolDefinition`, `generateCustomJobToolDefinition`, `generateMCPScriptsToolsConfig`).
+
+In the same change set we also adopt two narrower decisions for the other two lint findings: `updateManifestManagedWorkflow` is refactored to take a single `manifestManagedWorkflowUpdate` struct instead of nine positional parameters, and `extractWorkflowNameFromFile` is switched to named return values + a single deferred `Close` so the close error is propagated through the named `err` rather than racing the success path.
+
+### Alternatives Considered
+
+#### Alternative 1: Centralized helper (`mustAssert[T any](v any) (T, error)`)
+
+A single generic helper would have offered one canonical code shape at every site. We rejected it because the *recovery* behavior is what actually varies — half the sites need to fall back to a fresh value, a quarter need to delete a cache entry, and the rest need to either return or warn. A helper that only produced `(T, error)` would have forced every caller to reinvent its recovery logic anyway, and a helper rich enough to encode all four modes would be more complex than the inline two-value pattern.
+
+#### Alternative 2: Silence the linter with `//nolint:uncheckedtypeassertion`
+
+For internally-controlled caches we technically know the stored type. Suppressing the linter would have been the smallest diff. We rejected this because (a) the panic-on-mismatch failure mode is unacceptable for long-running processes — a compiler-pipeline panic blows up an entire workflow run — and (b) suppression hides the same class of bug from future contributors who modify cache-population sites without realizing a reader will panic on the new type.
+
+#### Alternative 3: Recover panics at the boundary
+
+We could have left the assertions unchecked and added `defer recover()` at job/analyzer boundaries. We rejected this because `recover` loses the static type information at the site, makes test-time failures harder to localize, and conflates type-invariant bugs with all other runtime panics.
+
+### Consequences
+
+#### Positive
+- Runtime type-invariant violations now surface as targeted errors or warnings instead of process-killing panics.
+- Self-healing caches (`unquoteYAMLKeyCache`, `builtinFrontmatterCache`) survive corruption without restart.
+- Linter remains green, making it a reliable gate for new code.
+- The struct-parameter refactor of `updateManifestManagedWorkflow` makes the call site at `pkg/cli/update_manifest.go` materially easier to read and extend.
+- File-close errors from `extractWorkflowNameFromFile` are now propagated even on the success path.
+
+#### Negative
+- Four recovery modes instead of one increases reviewer cognitive load — readers must check that the *right* mode was chosen for each site.
+- A few sites now silently degrade (e.g. dropping a schedule trigger with only a warning); a missed warning channel could mask real bugs.
+- Cache "re-store on type mismatch" introduces a small extra write on the corruption path; benign but not free.
+- The `manifestManagedWorkflowUpdate` struct adds a small layer of indirection at every field read inside `updateManifestManagedWorkflow`.
+
+#### Neutral
+- No public API change — all modifications are internal to `pkg/`.
+- The struct refactor is mechanically pure: parameter values are identical, only their packaging changes.
+- The defer-based close in `extractWorkflowNameFromFile` requires named return values, which is a style choice the rest of the package already uses.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Type Assertions in Library and Compiler Code
+
+1. New code in `pkg/` **MUST NOT** use the single-value form `v := x.(T)` for values read from `any`-typed containers (including `sync.Map`, `map[string]any`, and `analysis.Pass.ResultOf`).
+2. New code in `pkg/` **MUST** use the two-value form `v, ok := x.(T)` and explicitly handle the `!ok` branch when reading from any such container.
+3. The `!ok` branch **MUST** choose exactly one of: returning an error, deleting and reconstructing the cache entry, appending a structured warning and continuing with degraded output, or initializing a fresh container.
+4. Implementations **MUST NOT** suppress the `uncheckedtypeassertion` linter via `//nolint` comments to avoid handling the `!ok` case.
+5. Implementations **MAY** keep the single-value form for assertions against typed values obtained from `reflect` or where the static type is provably non-interface (e.g. concrete struct field access via type switch arms).
+
+### Function Parameter Lists
+
+1. Functions in `pkg/cli/` **SHOULD NOT** take more than the `excessivefuncparams` linter's configured limit of positional parameters.
+2. When the limit would be exceeded, implementations **SHOULD** group cohesive parameters into a single options or update struct, as done with `manifestManagedWorkflowUpdate`.
+
+### File Handles and Deferred Close
+
+1. Functions that `os.Open` a file **MUST** call `Close` via `defer` immediately after a successful open.
+2. Functions that need to propagate a close error on the success path **SHOULD** use named return values and assign the close error to the named `err` from within the deferred function.
+3. Implementations **MUST NOT** rely on a trailing non-deferred `Close` at the end of a function as the sole close path.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. The `uncheckedtypeassertion`, `excessivefuncparams`, and `fileclosenotdeferred` linters in `pkg/linters/` are the executable conformance check for the corresponding sections.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26491524257) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/jsonworkflow_to_markdown.go

@@ -445,14 +445,24 @@ func convertTriggersToOn(t *JSONWorkflowTriggers) (any, []string) {
 	// Single interval trigger → use the string shorthand directly.
 	intervalType, hasInterval := parts["_interval"]
 	if hasInterval && len(parts) == 1 {
-		return intervalType.(string), warnings
+		interval, ok := intervalType.(string)
+		if !ok {
+			warnings = append(warnings, "triggers.interval must resolve to a string value; skipped")
+			return nil, warnings
+		}
+		return interval, warnings
 	}
 
 	// Multiple triggers or non-interval: build a map.
 	// Convert interval shorthand to a schedule cron entry.
 	if hasInterval {
 		delete(parts, "_interval")
-		parts["schedule"] = []any{map[string]any{"cron": intervalToFuzzySchedule(intervalType.(string))}}
+		interval, ok := intervalType.(string)
+		if !ok {
+			warnings = append(warnings, "triggers.interval must resolve to a string value; schedule trigger skipped")
+		} else {
+			parts["schedule"] = []any{map[string]any{"cron": intervalToFuzzySchedule(interval)}}
+		}
 	}
 
 	return parts, warnings