fix: move changing runtime data into a validated catalog - #645
Conversation
rianjs
changed the title
3ac8aa3 to
8ad7485
Compare
Model catalog PR #645 — TDD coverage assessmentAssessment path: read BlockerNo findings. Major
Minor
NitNo findings. VerificationPassed focused suites:
|
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8ad7485cad52
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 0 |
| go:implementation-tests | 2 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
go:implementation-tests (2 findings)
Major - internal/cmd/initcmd/init_profile_v2.go:496
The profile editor still loses the selected snapshot. Both construction and syncModelMapFields obtain llm from initProfileEditorModelMapLLM, which creates a new LLMConfig without a catalog; document extraction likewise constructs a catalog-less LLMConfig. Consequently this lookup falls back to bundled defaults even under --catalog or an installed revision. If that revision changes the medium default, an explicit override equal to the bundled medium default is removed by normalizeInitModelMap as redundant, changing the saved effective model. Carry the selected catalog through editor construction, runtime switching, and document extraction, and test that editing preserves such an override against a catalog with different defaults.
Major - cmd/cr/main_test.go:206
The explicit-catalog test only runs catalog show against the unchanged bundled data. Root coverage proves pointer caching, and pipeline coverage directly calls the pricing helper with unchanged rates, but no behavioral test proves that command-selected catalog data reaches config resolution and execution consistently. These tests would pass if a command reverted to config.Load or runtime assembly dropped Catalog. Add a fixture with different defaults, effort/fast capabilities, and rates; exercise config resolution and the existing fake-backed review harness, change the installed revision after selection, and assert requests, override precedence, artifacts, and cost basis all use the original snapshot.
structure:repo-health (1 finding)
Major - internal/modelcatalog/catalog.go:888
Default validation checks model-supported efforts but never checks the owning runtime's maximum effort. For example, a catalog with the OpenAI runtime maximum changed to
highand its existing small-tier default left atmaxpasses validation and is published by Update, but stage resolution subsequently rejects that default through ValidateEffortForRuntime. This replaces a working installed snapshot with an internally inconsistent one despite the promised validation-before-publication boundary. Validate each default's effective effort against its runtime ceiling before publishing, and test that such an update fails while preserving the active pointer.
Reviewer Coverage
automation:ci-release— complete (broad); inspected 2 assigned files (48 inspected across reviewers):.goreleaser.yml,scripts/verify-package-render.sh; skipped: none; constraints: Review limited to the assigned packaging and verification changes, with supporting local automation context. Shell syntax validated; no full GoReleaser snapshot or live Homebrew installation was run.go:implementation-tests— complete (broad); inspected 45 assigned files (48 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/app/runtime.go,internal/app/runtime_selection.go,internal/app/runtime_test.go,internal/benchmark/suite.go,internal/cmd/agentscmd/agentscmd.go,internal/cmd/benchmarkcmd/benchmarkcmd.go,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/credentialcmd/credentialcmd.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/mecmd/mecmd.go,internal/cmd/respondcmd/respondcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/config/config.go,internal/config/config_test.go,internal/llmadapters/api.go,internal/llmadapters/api_test.go,internal/llmadapters/subprocess.go,internal/modelcatalog/catalog.go,internal/modelcatalog/catalog_test.go,internal/modelcatalog/data/defaults.csv,internal/modelcatalog/data/manifest.json,internal/modelcatalog/data/models.csv,internal/modelcatalog/data/pricing.csv,internal/modelcatalog/data/runtimes.csv,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go,internal/pipeline/prompts_test.go,internal/pricing/pricing.go,internal/pricing/pricing_test.go,internal/stagemodel/resolver.go,internal/stagemodel/resolver_test.go,internal/view/config_test.go; skipped: none; constraints: Initial tests encountered CGO build errors involving the workspace path and a sandbox restriction on httptest port binding. Focused catalog/model tests passed with CGO disabled and keyring_no1password. Review focused on Go implementation and behavioral coverage of catalog selection, overrides, refresh retention, and snapshot propagation.policies:conventions— complete (broad); inspected 21 assigned files (48 inspected across reviewers):.goreleaser.yml,cmd/cr/main.go,cmd/cr/main_test.go,docs/model-catalog-plan.md,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/config/config.go,internal/config/config_test.go,internal/modelcatalog/catalog.go,internal/pipeline/artifacts.go,scripts/verify-package-render.sh; skipped: none; constraints: Review limited to convention adherence in the assigned changes; tests were inspected but not executed. Shared standards and automation convenience copies were absent locally; fetching canonical shared docs failed. No missing policy contents were inferred.structure:repo-health— complete (broad); inspected 35 assigned files (48 inspected across reviewers):.goreleaser.yml,cmd/cr/main.go,docs/model-catalog-plan.md,internal/app/runtime.go,internal/app/runtime_selection.go,internal/benchmark/suite.go,internal/cmd/agentscmd/agentscmd.go,internal/cmd/benchmarkcmd/benchmarkcmd.go,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/credentialcmd/credentialcmd.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/mecmd/mecmd.go,internal/cmd/respondcmd/respondcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/config/config.go,internal/llmadapters/api.go,internal/llmadapters/subprocess.go,internal/modelcatalog/catalog.go,internal/modelcatalog/data/defaults.csv,internal/modelcatalog/data/manifest.json,internal/modelcatalog/data/models.csv,internal/modelcatalog/data/pricing.csv,internal/modelcatalog/data/runtimes.csv,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/pricing/pricing.go,internal/stagemodel/resolver.go,scripts/verify-package-render.sh; skipped: none; constraints: Focused tests were limited by sandbox denial of HTTP listener binding and a cgo compiler cache-path error. Pricing tests passed. Review focused on assigned files, snapshot ownership, validation boundaries, and refresh lifecycle.
Inspected files (48)
.goreleaser.ymlcmd/cr/main.gocmd/cr/main_test.godocs/model-catalog-plan.mdinternal/app/runtime.gointernal/app/runtime_selection.gointernal/app/runtime_test.gointernal/benchmark/suite.gointernal/cmd/agentscmd/agentscmd.gointernal/cmd/benchmarkcmd/benchmarkcmd.gointernal/cmd/catalogcmd/catalogcmd.gointernal/cmd/cmdruntime/cmdruntime.gointernal/cmd/configcmd/configcmd.gointernal/cmd/configcmd/configcmd_test.gointernal/cmd/credentialcmd/credentialcmd.gointernal/cmd/initcmd/init_llm_runtime_editor.gointernal/cmd/initcmd/init_profile_v2.gointernal/cmd/initcmd/initcmd.gointernal/cmd/initcmd/initcmd_test.gointernal/cmd/mecmd/mecmd.gointernal/cmd/respondcmd/respondcmd.gointernal/cmd/reviewcmd/review_defaults.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/root/root.gointernal/cmd/root/root_test.gointernal/config/config.gointernal/config/config_test.gointernal/llmadapters/api.gointernal/llmadapters/api_test.gointernal/llmadapters/subprocess.gointernal/modelcatalog/catalog.gointernal/modelcatalog/catalog_test.gointernal/modelcatalog/data/defaults.csvinternal/modelcatalog/data/manifest.jsoninternal/modelcatalog/data/models.csvinternal/modelcatalog/data/pricing.csvinternal/modelcatalog/data/runtimes.csvinternal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/prompts.gointernal/pipeline/prompts_test.gointernal/pricing/pricing.gointernal/pricing/pricing_test.gointernal/stagemodel/resolver.gointernal/stagemodel/resolver_test.gointernal/view/config_test.goscripts/verify-package-render.sh
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 00s | gpt-6.1-sol | cr 0.0.0-live
| Field | Value |
|---|---|
| Model | gpt-6.1-sol |
| Reviewers | automation:ci-release, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-sol |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 00s wall · 4m 47s compute |
| Cost | unavailable |
| Tokens | 2.2M in / 10.7k out |
Per-workstream usage
orchestrator-selection— gpt-6.1-sol- In: 38.0k
- Out: 1.5k
- Cache read: 24.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 53s
automation:ci-release— gpt-6.1-sol- In: 144.7k
- Out: 509
- Cache read: 122.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 18s
go:implementation-tests— gpt-6.1-sol- In: 1.0M
- Out: 3.5k
- Cache read: 921.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 34s
policies:conventions— gpt-6.1-sol- In: 266.5k
- Out: 918
- Cache read: 204.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 32s
structure:repo-health— gpt-6.1-sol- In: 623.9k
- Out: 2.6k
- Cache read: 544.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 15s
orchestrator-rollup— gpt-6.1-sol- In: 65.6k
- Out: 1.7k
- Cache read: 43.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 14s
| } | ||
| } | ||
|
|
||
| func TestRunCatalogShowUsesExplicitCatalogCapabilities(t *testing.T) { |
There was a problem hiding this comment.
The explicit-catalog test only runs catalog show against the unchanged bundled data. Root coverage proves pointer caching, and pipeline coverage directly calls the pricing helper with unchanged rates, but no behavioral test proves that command-selected catalog data reaches config resolution and execution consistently. These tests would pass if a command reverted to config.Load or runtime assembly dropped Catalog. Add a fixture with different defaults, effort/fast capabilities, and rates; exercise config resolution and the existing fake-backed review harness, change the installed revision after selection, and assert requests, override precedence, artifacts, and cost basis all use the original snapshot.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 0420fc5: added changed-catalog fake-adapter runtime coverage that removes its source and verifies request resolution, overrides, fast mode, artifacts, and pricing basis from one retained snapshot.
There was a problem hiding this comment.
Thanks—the changed-catalog runtime coverage and source removal address the retained-snapshot coverage gap described here.
Summary:
Addressed in 0420fc5 per the author's update: changed-catalog fake-adapter runtime coverage removes the catalog source after selection and checks request resolution, override precedence, fast mode, artifacts, and pricing against the retained snapshot.
| return fmt.Errorf("model catalog: default %s/%s model %q is not offered by runtime", value.RuntimeID, value.Tier, value.ModelID) | ||
| } | ||
| model := modelsByRuntime[value.RuntimeID][value.ModelID] | ||
| if !contains(model.SupportedEfforts, value.Effort) { |
There was a problem hiding this comment.
Default validation checks model-supported efforts but never checks the owning runtime's maximum effort. For example, a catalog with the OpenAI runtime maximum changed to high and its existing small-tier default left at max passes validation and is published by Update, but stage resolution subsequently rejects that default through ValidateEffortForRuntime. This replaces a working installed snapshot with an internally inconsistent one despite the promised validation-before-publication boundary. Validate each default's effective effort against its runtime ceiling before publishing, and test that such an update fails while preserving the active pointer.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 859ccc9: catalog validation now checks each default against the owning runtime maximum effort before publication, preserving the active pointer on failure.
There was a problem hiding this comment.
Thanks—checking default efforts against the owning runtime ceiling before publication and preserving the active pointer on failure addresses this finding.
Summary:
Addressed in 859ccc9 per the author's update: catalog validation checks each default against its owning runtime's maximum effort before publication, preserving the active pointer when validation fails.
| existing := copyModelMap(modelMap) | ||
| effective := config.EffectiveModelMap(applyModelMapToLLM(llm, existing)) | ||
| builtIns := config.BuiltInModelMap(llm.Provider, llm.Adapter) | ||
| builtIns := config.BuiltInModelMapFor(llm.Catalog(), llm.Provider, llm.Adapter) |
There was a problem hiding this comment.
The profile editor still loses the selected snapshot. Both construction and syncModelMapFields obtain llm from initProfileEditorModelMapLLM, which creates a new LLMConfig without a catalog; document extraction likewise constructs a catalog-less LLMConfig. Consequently this lookup falls back to bundled defaults even under --catalog or an installed revision. If that revision changes the medium default, an explicit override equal to the bundled medium default is removed by normalizeInitModelMap as redundant, changing the saved effective model. Carry the selected catalog through editor construction, runtime switching, and document extraction, and test that editing preserves such an override against a catalog with different defaults.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 859ccc9: init profile editor construction, switching, and extraction now carry the selected catalog snapshot; regression coverage preserves explicit overrides.
There was a problem hiding this comment.
Thanks—carrying the selected snapshot through editor construction, switching, and extraction, with regression coverage for explicit overrides, addresses this finding.
Summary:
Addressed in 859ccc9 per the author's update: init profile editor construction, runtime switching, and document extraction retain the selected catalog snapshot, with regression coverage preserving explicit overrides.
8ad7485 to
859ccc9
Compare
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 859ccc9064cf
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 0 |
| go:implementation-tests | 2 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
go:implementation-tests (2 findings)
Major - internal/benchmark/suite.go:471
The tier-based validation discards exactly the model-effort error it needs to report. ResolveStageModel already calls ValidateEffortForModel; when the requested effort is unsupported, err is non-nil, model remains empty, and the subsequent ValidateEffortForModel returns nil for that empty model. For example, an OpenAI API profile mapping medium to gpt-5.4 accepts a reviewer candidate with model_tier: medium and effort: max, while the equivalent explicit-model candidate is rejected. The invalid suite can therefore proceed into benchmark execution before failing at reviewer resolution. Propagate the resolver error with candidate/stage context and ErrInvalid, and add a validation test covering this tier-based unsupported-effort case.
Major - internal/cmd/reviewcmd/reviewcmd_test.go:122
This test now proves command-to-request snapshot propagation, but it still bypasses the execution contract identified in the previous review. fakeRunner returns a preconstructed summary containing the expected model, cost, and revision; the pricing assertion separately invokes EstimateUsageUSDFor. No pipeline stage resolves the changed default or validates the fixture's fast capability, and no artifact or summary is produced from actual fake-adapter usage. Removing Catalog from app's pipeline.Options would therefore leave this test passing while execution loses the selected pricing and artifact revision. Extend the existing fake-adapter pipeline/runtime harness with this distinct catalog, remove or replace its source after loading, and assert resolved requests, effective fast mode, generated catalog metadata, and computed cost basis.
Reviewer Coverage
automation:ci-release— complete (constrained); inspected 2 assigned files (49 inspected across reviewers):.goreleaser.yml,scripts/verify-package-render.sh; skipped: none; constraints: Review limited to the assigned packaging and verification changes, with supporting local automation context. Shell syntax validated; read-only review did not run a full GoReleaser snapshot or live Homebrew installation.go:implementation-tests— complete (constrained); inspected 46 assigned files (49 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/app/runtime.go,internal/app/runtime_selection.go,internal/app/runtime_test.go,internal/benchmark/suite.go,internal/cmd/agentscmd/agentscmd.go,internal/cmd/benchmarkcmd/benchmarkcmd.go,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/credentialcmd/credentialcmd.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/mecmd/mecmd.go,internal/cmd/respondcmd/respondcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/config/config.go,internal/config/config_test.go,internal/llmadapters/api.go,internal/llmadapters/api_test.go,internal/llmadapters/subprocess.go,internal/modelcatalog/catalog.go,internal/modelcatalog/catalog_test.go,internal/modelcatalog/data/defaults.csv,internal/modelcatalog/data/manifest.json,internal/modelcatalog/data/models.csv,internal/modelcatalog/data/pricing.csv,internal/modelcatalog/data/runtimes.csv,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go,internal/pipeline/prompts_test.go,internal/pricing/pricing.go,internal/pricing/pricing_test.go,internal/stagemodel/resolver.go,internal/stagemodel/resolver_test.go,internal/view/config_test.go; skipped: none; constraints: Focused coverage repair for the 27 assigned files; primary review findings were excluded. Follow-up review focused on fixes for the previous findings and snapshot propagation through review execution; other assigned files were not re-inspected. Read-only sandbox prevented running Go tests requiring build and temporary-file writes. Static review only: the read-only sandbox prevents Go test build and temporary-file writes. The previous head commit is unavailable in this checkout, so inspected changes were compared with the PR base.policies:conventions— complete (constrained); inspected 21 assigned files (49 inspected across reviewers):.goreleaser.yml,cmd/cr/main.go,cmd/cr/main_test.go,docs/model-catalog-plan.md,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/config/config.go,internal/config/config_test.go,internal/modelcatalog/catalog.go,internal/pipeline/artifacts.go,scripts/verify-package-render.sh; skipped: none; constraints: Review focused on convention adherence in the assigned changes; tests were inspected but not executed in the read-only environment. Shared standards and automation convenience copies were absent locally, and canonical shared documentation fetches failed. Missing policy contents were not inferred.structure:repo-health— complete (constrained); inspected 35 assigned files (49 inspected across reviewers):.goreleaser.yml,cmd/cr/main.go,docs/model-catalog-plan.md,internal/app/runtime.go,internal/app/runtime_selection.go,internal/benchmark/suite.go,internal/cmd/agentscmd/agentscmd.go,internal/cmd/benchmarkcmd/benchmarkcmd.go,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/credentialcmd/credentialcmd.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/mecmd/mecmd.go,internal/cmd/respondcmd/respondcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/config/config.go,internal/llmadapters/api.go,internal/llmadapters/subprocess.go,internal/modelcatalog/catalog.go,internal/modelcatalog/data/defaults.csv,internal/modelcatalog/data/manifest.json,internal/modelcatalog/data/models.csv,internal/modelcatalog/data/pricing.csv,internal/modelcatalog/data/runtimes.csv,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/pricing/pricing.go,internal/stagemodel/resolver.go,scripts/verify-package-render.sh; skipped: none; constraints: Static review only; tests were not executed in the read-only environment. The previous review SHA was unavailable locally; reviewed the current head against the supplied base and rechecked earlier findings.
Inspected files (49)
.goreleaser.ymlcmd/cr/main.gocmd/cr/main_test.godocs/model-catalog-plan.mdinternal/app/runtime.gointernal/app/runtime_selection.gointernal/app/runtime_test.gointernal/benchmark/suite.gointernal/cmd/agentscmd/agentscmd.gointernal/cmd/benchmarkcmd/benchmarkcmd.gointernal/cmd/catalogcmd/catalogcmd.gointernal/cmd/cmdruntime/cmdruntime.gointernal/cmd/configcmd/configcmd.gointernal/cmd/configcmd/configcmd_test.gointernal/cmd/credentialcmd/credentialcmd.gointernal/cmd/initcmd/init_llm_runtime_editor.gointernal/cmd/initcmd/init_profile_v2.gointernal/cmd/initcmd/initcmd.gointernal/cmd/initcmd/initcmd_test.gointernal/cmd/mecmd/mecmd.gointernal/cmd/respondcmd/respondcmd.gointernal/cmd/reviewcmd/review_defaults.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/cmd/root/root.gointernal/cmd/root/root_test.gointernal/config/config.gointernal/config/config_test.gointernal/llmadapters/api.gointernal/llmadapters/api_test.gointernal/llmadapters/subprocess.gointernal/modelcatalog/catalog.gointernal/modelcatalog/catalog_test.gointernal/modelcatalog/data/defaults.csvinternal/modelcatalog/data/manifest.jsoninternal/modelcatalog/data/models.csvinternal/modelcatalog/data/pricing.csvinternal/modelcatalog/data/runtimes.csvinternal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/prompts.gointernal/pipeline/prompts_test.gointernal/pricing/pricing.gointernal/pricing/pricing_test.gointernal/stagemodel/resolver.gointernal/stagemodel/resolver_test.gointernal/view/config_test.goscripts/verify-package-render.sh
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 01s | gpt-6.1-sol | cr 0.0.0-live
| Field | Value |
|---|---|
| Model | gpt-6.1-sol |
| Reviewers | automation:ci-release, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-sol |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 2m 01s wall · 2m 47s compute |
| Cost | unavailable |
| Tokens | 6.5M in / 20.1k out |
Per-workstream usage
automation:ci-release— gpt-6.1-sol- In: 224.6k
- Out: 738
- Cache read: 173.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 9s
go:implementation-tests— gpt-6.1-sol- In: 4.2M
- Out: 11.6k
- Cache read: 3.7M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 23s
policies:conventions— gpt-6.1-sol- In: 734.4k
- Out: 1.9k
- Cache read: 575.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 28s
structure:repo-health— gpt-6.1-sol- In: 1.3M
- Out: 4.0k
- Cache read: 1.1M
- Cache create: unavailable
- Cost: unavailable
- Duration: 37s
orchestrator-rollup— gpt-6.1-sol- In: 98.1k
- Out: 1.9k
- Cache read: 55.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 8s
|
|
||
| in, outTokens := 1_000_000, 1_000_000 | ||
| cost := 10.0 | ||
| runner := &fakeRunner{result: testPipelineResult(false)} |
There was a problem hiding this comment.
This test now proves command-to-request snapshot propagation, but it still bypasses the execution contract identified in the previous review. fakeRunner returns a preconstructed summary containing the expected model, cost, and revision; the pricing assertion separately invokes EstimateUsageUSDFor. No pipeline stage resolves the changed default or validates the fixture's fast capability, and no artifact or summary is produced from actual fake-adapter usage. Removing Catalog from app's pipeline.Options would therefore leave this test passing while execution loses the selected pricing and artifact revision. Extend the existing fake-adapter pipeline/runtime harness with this distinct catalog, remove or replace its source after loading, and assert resolved requests, effective fast mode, generated catalog metadata, and computed cost basis.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 0420fc5: actual app runtime and fake-adapter coverage now exercises changed defaults, capabilities, pricing, source removal, resolved requests, fast metadata, generated artifacts, and computed cost basis.
There was a problem hiding this comment.
Thanks—the actual runtime and fake-adapter coverage with source removal, generated artifacts, and computed costs addresses the execution coverage gap.
Summary:
Addressed in 0420fc5 per the author's update: actual app runtime and fake-adapter coverage exercises changed catalog defaults, capabilities, and pricing after source removal, checking resolved requests, fast metadata, generated artifacts, and computed cost basis.
| Tier: config.ModelTier(candidate.Stages.Reviewers.ModelTier), | ||
| EffortOverride: stage.effort, | ||
| }) | ||
| if err == nil { |
There was a problem hiding this comment.
The tier-based validation discards exactly the model-effort error it needs to report. ResolveStageModel already calls ValidateEffortForModel; when the requested effort is unsupported, err is non-nil, model remains empty, and the subsequent ValidateEffortForModel returns nil for that empty model. For example, an OpenAI API profile mapping medium to gpt-5.4 accepts a reviewer candidate with model_tier: medium and effort: max, while the equivalent explicit-model candidate is rejected. The invalid suite can therefore proceed into benchmark execution before failing at reviewer resolution. Propagate the resolver error with candidate/stage context and ErrInvalid, and add a validation test covering this tier-based unsupported-effort case.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 0420fc5: benchmark tier resolution now propagates unsupported model-effort errors with ErrInvalid candidate and stage context, with regression coverage.
There was a problem hiding this comment.
Thanks—propagating the resolver error with ErrInvalid and candidate/stage context, plus regression coverage, addresses this finding.
Summary:
Addressed in 0420fc5 per the author's update: benchmark tier resolution propagates unsupported model-effort errors with ErrInvalid and candidate/stage context, with regression coverage.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 0420fc505067
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 0 |
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
automation:ci-release— complete (constrained); inspected 2 assigned files (50 inspected across reviewers):.goreleaser.yml,scripts/verify-package-render.sh; skipped: none; constraints: Review limited to the assigned packaging and verification changes, with supporting local automation context. Shell syntax validated; read-only review did not run a full GoReleaser snapshot or live Homebrew installation.go:implementation-tests— complete (constrained); inspected 47 assigned files (50 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/app/runtime.go,internal/app/runtime_selection.go,internal/app/runtime_test.go,internal/benchmark/suite.go,internal/benchmark/suite_test.go,internal/cmd/agentscmd/agentscmd.go,internal/cmd/benchmarkcmd/benchmarkcmd.go,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/credentialcmd/credentialcmd.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/mecmd/mecmd.go,internal/cmd/respondcmd/respondcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/config/config.go,internal/config/config_test.go,internal/llmadapters/api.go,internal/llmadapters/api_test.go,internal/llmadapters/subprocess.go,internal/modelcatalog/catalog.go,internal/modelcatalog/catalog_test.go,internal/modelcatalog/data/defaults.csv,internal/modelcatalog/data/manifest.json,internal/modelcatalog/data/models.csv,internal/modelcatalog/data/pricing.csv,internal/modelcatalog/data/runtimes.csv,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go,internal/pipeline/prompts_test.go,internal/pricing/pricing.go,internal/pricing/pricing_test.go,internal/stagemodel/resolver.go,internal/stagemodel/resolver_test.go,internal/view/config_test.go; skipped: none; constraints: Incremental review reused prior inspections of unchanged files; git comparison with 859ccc9 confirmed only runtime_test.go, suite.go, and suite_test.go changed. Static review only; tests were not rerun in the read-only sandbox.policies:conventions— complete (constrained); inspected 21 assigned files (50 inspected across reviewers):.goreleaser.yml,cmd/cr/main.go,cmd/cr/main_test.go,docs/model-catalog-plan.md,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/config/config.go,internal/config/config_test.go,internal/modelcatalog/catalog.go,internal/pipeline/artifacts.go,scripts/verify-package-render.sh; skipped: none; constraints: Incremental review: all assigned files are unchanged from previously reviewed 859ccc9. Verified this against 0420fc5 and inspected the new runtime coverage as supporting context. Shared policy copies remain absent locally; canonical shared documentation fetches failed in prior review turns. Missing policy contents were not inferred. Tests were not executed in the read-only environment.structure:repo-health— complete (constrained); inspected 35 assigned files (50 inspected across reviewers):.goreleaser.yml,cmd/cr/main.go,docs/model-catalog-plan.md,internal/app/runtime.go,internal/app/runtime_selection.go,internal/benchmark/suite.go,internal/cmd/agentscmd/agentscmd.go,internal/cmd/benchmarkcmd/benchmarkcmd.go,internal/cmd/catalogcmd/catalogcmd.go,internal/cmd/cmdruntime/cmdruntime.go,internal/cmd/configcmd/configcmd.go,internal/cmd/credentialcmd/credentialcmd.go,internal/cmd/initcmd/init_llm_runtime_editor.go,internal/cmd/initcmd/init_profile_v2.go,internal/cmd/initcmd/initcmd.go,internal/cmd/mecmd/mecmd.go,internal/cmd/respondcmd/respondcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/root/root.go,internal/config/config.go,internal/llmadapters/api.go,internal/llmadapters/subprocess.go,internal/modelcatalog/catalog.go,internal/modelcatalog/data/defaults.csv,internal/modelcatalog/data/manifest.json,internal/modelcatalog/data/models.csv,internal/modelcatalog/data/pricing.csv,internal/modelcatalog/data/runtimes.csv,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/pricing/pricing.go,internal/stagemodel/resolver.go,scripts/verify-package-render.sh; skipped: none; constraints: Catalog data was inspected for structural consistency; published capabilities and prices were not independently verified. Focused on the 27 coverage-repair files and relevant dependency context; earlier settled findings were not repeated. Incremental review against previously reviewed 859ccc9, with targeted reinspection of snapshot propagation and earlier fixes. Unchanged assigned files were not rereviewed. Static review only; tests and package rendering were not executed in the read-only environment. Static review only; tests were not executed in the read-only environment. Added runtime and benchmark regression tests were inspected.
Inspected files (50)
.goreleaser.ymlcmd/cr/main.gocmd/cr/main_test.godocs/model-catalog-plan.mdinternal/app/runtime.gointernal/app/runtime_selection.gointernal/app/runtime_test.gointernal/benchmark/suite.gointernal/benchmark/suite_test.gointernal/cmd/agentscmd/agentscmd.gointernal/cmd/benchmarkcmd/benchmarkcmd.gointernal/cmd/catalogcmd/catalogcmd.gointernal/cmd/cmdruntime/cmdruntime.gointernal/cmd/configcmd/configcmd.gointernal/cmd/configcmd/configcmd_test.gointernal/cmd/credentialcmd/credentialcmd.gointernal/cmd/initcmd/init_llm_runtime_editor.gointernal/cmd/initcmd/init_profile_v2.gointernal/cmd/initcmd/initcmd.gointernal/cmd/initcmd/initcmd_test.gointernal/cmd/mecmd/mecmd.gointernal/cmd/respondcmd/respondcmd.gointernal/cmd/reviewcmd/review_defaults.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/cmd/root/root.gointernal/cmd/root/root_test.gointernal/config/config.gointernal/config/config_test.gointernal/llmadapters/api.gointernal/llmadapters/api_test.gointernal/llmadapters/subprocess.gointernal/modelcatalog/catalog.gointernal/modelcatalog/catalog_test.gointernal/modelcatalog/data/defaults.csvinternal/modelcatalog/data/manifest.jsoninternal/modelcatalog/data/models.csvinternal/modelcatalog/data/pricing.csvinternal/modelcatalog/data/runtimes.csvinternal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/prompts.gointernal/pipeline/prompts_test.gointernal/pricing/pricing.gointernal/pricing/pricing_test.gointernal/stagemodel/resolver.gointernal/stagemodel/resolver_test.gointernal/view/config_test.goscripts/verify-package-render.sh
5 PR discussion threads considered. 5 summarized; 5 resolved.
Completed in 1m 40s | gpt-6.1-sol | cr dev
| Field | Value |
|---|---|
| Model | gpt-6.1-sol |
| Reviewers | automation:ci-release, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-sol |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 40s wall · 2m 05s compute |
| Cost | unavailable |
| Tokens | 9.2M in / 22.8k out |
Per-workstream usage
automation:ci-release— gpt-6.1-sol- In: 318.4k
- Out: 958
- Cache read: 259.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 10s
go:implementation-tests— gpt-6.1-sol- In: 3.5M
- Out: 7.7k
- Cache read: 3.2M
- Cache create: unavailable
- Cost: unavailable
- Duration: 33s
policies:conventions— gpt-6.1-sol- In: 1.1M
- Out: 2.5k
- Cache read: 920.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 19s
structure:repo-health— gpt-6.1-sol- In: 4.1M
- Out: 10.8k
- Cache read: 3.4M
- Cache create: unavailable
- Cost: unavailable
- Duration: 53s
orchestrator-rollup— gpt-6.1-sol- In: 153.5k
- Out: 901
- Cache read: 125.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 8s
1 participant
Summary
Move changing runtime capabilities, defaults, and price observations into a validated, revisioned catalog with an offline baseline and atomic refreshes.
The command path now selects one immutable catalog snapshot and carries it through configuration, initialization, selection, execution, adapters, artifacts, and pricing. User overrides retain precedence, unknown explicit models remain standard-speed usable, and transport-specific speed behavior remains in the adapters.
Validation
make checkmake buildmake snapshot