Repository navigation
fix(usai-catalog): preserve and validate catalog role recommendations - #553
Conversation
…ations schema catalog.schema.json restricted recommendations to primary/small/compaction while the shipped catalog.json and the OpenCode renderer also use summary and title. Add both roles and a schema-level test (real JSON Schema validator) so schema, data, and renderer vocabulary stay aligned. Co-authored-by: OpenCode Agent <user@gsa.gov>
A write-mode build of the USAi catalog neither produced nor kept the `recommendations` block, so refreshing the catalog silently deleted the role defaults that the OpenCode kit renders. The block also serialized in a different position than the committed file, the validator did not check role names or that the ids exist, and the drift check could not see a change. - Serialize `recommendations` after `sources`, with roles in a fixed order, so re-serializing the committed catalog is byte-identical. - validateCatalog: roles must come from the schema, and every value must name a model in models[]. - Regeneration carries the existing recommendations forward. If a recommended model is no longer in the model list, or the existing catalog cannot be read as JSON, the build fails before writing and a person re-picks. There is no automatic selection. - describeDrift names recommendation changes. - Document the behavior in the catalog README. catalog.json is unchanged. Co-authored-by: OpenCode Agent <bret.amogilefsky@gsa.gov>
basiliskus
left a comment
basiliskus
left a comment
There was a problem hiding this comment.
Verdict: request changes — c8e9b8f2c1a7962d26e7711cab8261f049b1e898
readExistingRecommendations() treats any valid JSON without a recommendations key as if no existing catalog exists. I reproduced this with an output file containing []: write mode exited successfully, replaced it with a 24-model catalog, and omitted recommendations. That contradicts the fail-closed contract at integrations/providers/usai/scripts/build-catalog.mjs:1162-1164 and can still silently erase the curated role defaults this PR is intended to preserve. Please validate that an existing output is actually a catalog (or minimally a plain object with the expected catalog identity/shape) before carrying recommendations forward.
Minor: scripts/tests/test_usai_catalog_schema.py:3-4 says the hand-written validator does not enforce recommendation role names, but this PR now adds that enforcement.
Assisted-by: OpenCode [usai/gpt-5.6-sol-latest-guardrails-defaultv2] skill:change-review
…mendations-contract
readExistingRecommendations treated any valid JSON without a `recommendations` key as "no catalog yet", so a write-mode build over a file containing `[]` replaced it with a role-less catalog and exited 0. That defeated the fail-closed carry-forward contract. The file at the output path must now be absent or a real catalog (object, `usai-model-catalog/v1`, models[] array). Anything else fails before any write and leaves the file untouched. A real catalog that has no `recommendations` yet is still accepted and documented as such. Also correct the schema test docstring: the hand-written validator now enforces recommendation role names and referential ids. Co-authored-by: OpenCode Agent <bret.amogilefsky@gsa.gov>
|
🤖: AI agent (First Mate), posting on behalf of @mogul. Reply to the review at Fixed in |
basiliskus
left a comment
basiliskus
left a comment
There was a problem hiding this comment.
Verdict: approve — 9f9263486f3d87562a6ade8256f43bf9da3056d6
The previous non-catalog overwrite finding is resolved: eight regression cases fail on the prior head and pass here, including byte-preservation assertions. The validator docstring finding is also resolved. No new findings.
Nit: integrations/providers/usai/README.md:48-49 permits editing recommendations while line 15 forbids all hand-editing. Qualify line 15: “Model data is generated; only curated recommendations are hand-edited.”
Live USAi refresh was not exercised. CI job/step statuses are successful at this head; raw logs were unavailable.
Assisted-by: OpenCode [usai/gpt-6.1-sol-latest-guardrails-defaultv2] skill:change-review
🤖: AI agent (First Mate), AI-assisted change (OpenCode). Human review required before merge.
Summary
Part of #419 (slice S1). Makes the USAi catalog's
recommendations(role defaults) a validated, preserved, deterministic part of the catalog pipeline, so a catalog refresh can no longer silently delete them.catalog.jsonis unchanged.Eligibility & Accountability
AI Assistance Disclosure
Type of Change
Context
Found in the #419 audit and re-verified on main@7dd2ddc0:
build-catalog.mjsneither produced nor preservedrecommendations. A bootstrap run to a temp file dropped the block, and a fixture refresh did the same.recommendationswas missing fromKEY_ORDER, soserialize(JSON.parse(catalog.json))did not reproduce the committed file.validateCatalogdid not check role names or that role ids exist inmodels[]. A dangling id plus a made-up role produced 0 errors.describeDriftignoredrecommendations.summaryandtitle. That is fixed by fix(usai-catalog): allow summary and title roles in catalog recommendations schema #551, not here.Plan (what changed)
scripts/build-catalog.mjsKEY_ORDER:recommendationsaftersources, roles in fixed order.validateCatalog: role names come fromschema.properties.recommendations.properties; each value must be a non-empty string naming a model inmodels[]; guards against non-object blocks.readExistingRecommendationsreads the catalog at--out;carryForwardRecommendationskeeps the values and fails closed. Applies to the live/fixture and bootstrap paths and to--check.describeDrift: reportsrecommendations.<role>: "old" -> "new".tests/recommendations.test.mjs: 31 offline tests (fixtures only, temp output paths, no network).README.md: new "Role defaults" section.--outmust be absent or a real catalog (object,schemaVersionusai-model-catalog/v1,modelsarray).[],null,123,"text",{}, a wrongschemaVersion, or a catalog withoutmodels[]now fail before any write and leave the file untouched. Previously valid JSON without arecommendationskey was read as "no catalog yet" and overwritten. A real catalog with norecommendationsyet is still accepted (no roles to lose; roles are only added by a person); the shipped catalog always has them.scripts/tests/test_usai_catalog_schema.py: docstring corrected, because the hand-written validator now enforces role names and referential ids.Blast radius: 4 files.
Decisions recorded (owner, binding for later slices)
catalog.jsonis unchanged.From PR 495 this carries only fail-closed referential validation of role ids against
models[]. Deterministic key order was already mostly inserialize; this finishes it forrecommendations.Verification
Local run on this branch (head 6ffada1):
catalog.jsonre-serialization byte-identicalcatalog.jsonmodified in this PRnode scripts/build-catalog.mjs --checkmake validatemake test-kits(opencode 62, usai-provider 18, paseo 65, providers/usai 153)PYTHONPATH=scripts pytest scripts/testsruff check/ruff format --checkonscripts/make validate-kits,make validate-provider-input-contract,make generate-checkIndependent read-only AI reviews (blockers only): NO BLOCKERS on the original change, NO BLOCKERS on the merge of main plus the review fix. Non-blocking notes: with
--checkthe role lines indescribeDriftare reachable only through a direct call, because carry-forward copies the existing roles into the fresh catalog; the non-catalog tests exercise write mode, and--checkshares the same code path.CI results: see the checks on this PR.
Rollback
Revert the commit.
catalog.jsonis unchanged, so no data needs restoring.Security Impact
None to authentication, authorization or attack surface. No credential handling changed and no secret value appears in the diff. The carry-forward only reads and writes the catalog path the build already uses. The fail-closed paths reduce the chance of shipping a catalog with missing defaults.
Related: #357.