Confine catalog-ID coupling to the dispatch layer

[jmeridth]

🤖 Raised during review of #446; the pattern predates that PR and now spans most recently added checks.

Background

The intended design binds catalog requirement IDs to logic in exactly one place: the dispatch map in evaluation_plans/evaluation-plans.go (ID to step function), with catalog content in data/catalogs/*.yaml and step functions staying catalog-agnostic so they can be reused by other catalogs or other checks. That structure still exists, but catalog-ID coupling has drifted into the step packages themselves in three forms.

1. Doc comments tied to requirement IDs (cosmetic, ~24 instances)

Most recently added steps are documented as implementations of one specific requirement, for example:

• sec_assessment/steps.go:317 // HasThreatModelAnalysis implements OSPS-SA-03.02: ...
• build_release/steps.go:140 // CicdUntrustedCodeIsolation checks OSPS-BR-01.03: ...
• vuln_management/steps.go:578 // SastEnforcedOnChanges implements OSPS-VM-06.02: ...
• access_control/steps.go:240 // WorkflowJobPermissionsLeastPrivilege implements OSPS-AC-04.02: ...

Older checks (legal, docs, early governance) describe behavior without IDs. Cleanup: reword step doc comments to describe the behavior being assessed; the ID linkage lives in the dispatch map, which is already keyed by ID.

2. Hardcoded catalog IDs in runtime code (substantive, ~12 instances)

Every AI-assisted step passes its own requirement ID as a string literal into shared helpers, and the ID flows into log output:

• quality/steps.go: AIFallback(payload, "OSPS-QA-06.02", ...), recordAIAssessment(payload, "OSPS-QA-06.03", ...)
• access_control/steps.go: AIFallback(payload, "OSPS-AC-04.02", ...)

Root cause: the TypedStep signature does not provide the invoking requirement ID at runtime, even though the dispatch layer knows it, so authors inlined it. Comment cleanup alone leaves this coupling in place. Proposed fix: thread the requirement ID from the dispatch layer into steps that need it for logging/evidence (for example a closure wrapper at registration, or SDK support in pluginkit), and remove the string literals.

3. Minor naming

• vuln_management/vm_05_test.go is named after a requirement family rather than the behavior it tests.

Proposed cleanup pass

• Reword step doc comments to behavior descriptions (category 1)
• Thread the requirement ID from the dispatch layer and delete hardcoded ID literals (category 2)
• Rename vm_05_test.go (category 3)
• Consolidate the three release-asset companion/signature classifiers (build_release signatureAssetKind/isHashManifest/isReleaseAssetCompanion, quality compiledReleaseAssetExtensions and SBOM/signature suffixes) into reusable_steps (noted in the feat(OSPS-BR-02.02): associate release assets with release identifiers #445 review)
• Extract a shared fence-aware markdown section helper (consumers: vuln_management documentationSections, governance escalationPolicySections; noted in the feat(OSPS-GV-04.01): require review before escalated permissions #446 review)
• Add a CI guard so the drift does not return, e.g. fail if OSPS- appears in evaluation_plans/ outside evaluation-plans.go (allowing data/catalogs/, badgeurl, and docs)
• Reword the 14 catalog-ID references in data/ comments (payload.go 2, graphql-binary-check.go 6, rest-data.go 5, vex.go 1, vuln_reporting.go 1), and decide whether the CI guard should cover that directory too. The payload layer gathers GitHub data and is not catalog-specific, so naming controls there is the same drift.

Sequencing

Open PRs #445, #446, and #447 originally followed the drifted style described above. Their new doc comments have since been reworded to the target convention (e922b1f, 7234d5a, ecab5d1), so merging them will not add to the cleanup surface. The remaining pass covers the pre-existing code on main (categories 1 through 3) plus the CI guard.

[eddie-knight]

🤖 Worked through this against upstream/main (57f2aeb). Four findings that change the shape of the cleanup, plus a scope item added to the checklist above.

Category 2 cannot be fixed plugin-side

The proposed alternative — "a closure wrapper at registration" — does not work. pluginkit.adaptTypedSteps resolves each step's name by calling FuncName on the value in the step map, and a closure erases it. Tested against the real SDK at v1.32.2:

// binding the dispatch key in a wrapper, as AllSteps() would
for id, fn := range idSteps {
    out[id] = func(p Payload) (gemara.Result, string, gemara.ConfidenceLevel) { return fn(p, id) }
}

direct symbols (today):  main.StepAlpha  /  main.StepBeta
loop-bound closures:     main.AllSteps.func1  /  main.AllSteps.func1

Both steps collapse to one symbol. That is exactly the regression typed_step.go:14-18 was written to prevent — the names would disappear from the benchmark report and the evaluation log. (A per-call-site wrapper did keep distinct names, but only because the compiler inlined it. That is an optimization artifact, not a contract.)

So the only correct fix is SDK-side, which means an SDK release lands before the plugin PR can merge, since a replace directive cannot be committed. Recommend closing category 2 as won't-fix for now and leaving the twelve literals in place.

The cost of leaving them is smaller than the issue implies. The ID's entire runtime role is one log-line prefix, in reusable_steps/steps.go:96:

payload.Config.Logger.Warn(controlID+": "+reason, "err", err)

It never reaches evidence or the evaluation log. And the SDK already emits <requirementId>: <message> for every assessment (evaluation_suite.go:94), so the ID is duplicated context on that line rather than unique information.

There is a cheaper fix available if this becomes worth doing before the SDK changes: fold the reason into the returned message and delete the plugin-side Warn entirely. The SDK's own line then carries both ID and reason, the controlID parameter disappears, and one duplicate log line goes away. The trade-off is that the failure reason becomes user-visible result text rather than log-only.

Category 1 is 19 instances, not 24

Five of the 24 are cross-references to sibling steps, not self-declarations:

Site	Reference
build_release/steps.go:230	the tiered OSPS-BR-01.03 verdict documented on CicdUntrustedCodeIsolation
build_release/steps.go:908	Unlike OSPS-BR-01.01's fixed list…
build_release/steps.go:928	by CicdSanitizedInputParameters (OSPS-BR-01.01)
vuln_management/steps.go:626	applies the OSPS-VM-06.02 decision matrix
quality/steps.go:678	as AI input for OSPS-QA-06.02

These carry information a behavior description cannot. The right edit is to name the sibling function — which a reader can navigate to — rather than delete the pointer.

A fourth category: IDs inside AI prompt text

Not listed in the issue. Three prompt constants open by naming the requirement they serve, and two golden files mirror them:

• quality/steps.go:822, quality/steps.go:844, access_control/steps.go:577
• quality/testdata/documents_test_maintenance_policy_prompt.golden, access_control/testdata/workflow_job_permissions_prompt.golden

A grep OSPS- guard flags all five. Worth noting these are the one place the ID is genuinely useless: each prompt restates the requirement in full immediately after the ID, and the model has no catalog to resolve the identifier against.

The CI guard has to be a budget, not a ban

With category 2 left in place, a guard that fails on any OSPS- outside evaluation-plans.go fails on day one. What works is recording the twelve remaining literals as a per-file count and failing in both directions — a new ID fails, and so does a removed one, so the count shrinks as the coupling is paid down instead of silently authorizing a replacement.

Two refinements worth building in:

• Match the ID shape ([A-Z]{2,6}-[A-Z]{2}-[0-9]{2}\.[0-9]{2}) rather than the literal OSPS-, so a second catalog cannot reintroduce the same coupling under a different prefix.
• Exempt test files. Naming the requirement under test says what a fixture is for, and no catalog binding escapes into shipped behavior through one.

A Go test does this better than a shell script in a workflow: it runs under make test and the existing build.yaml, needs no new CI wiring, and is cross-platform.

On the shared section helper

Confirmed extractable — governance.escalationPolicySections and vuln_management.documentationSections have identical scanning rules and differ only in what they do to the text afterward, so the normalization stays caller-side.

One behavior change to be aware of: governance currently emits whitespace-only sections and vuln_management does not. Unifying on the stricter rule is safe — classifyEscalationSection returns (false, false) for all three shapes that behavior produces, so no verdict moves.