Skip to content

fix(groq): replace retired models with openai/gpt-oss-120b - #161

Merged
koydas merged 3 commits into
mainfrom
claude/fix-guard-hardening-fuobch
Oct 1, 2026
Merged

koydas merged 3 commits into
mainfrom
claude/fix-guard-hardening-fuobch

Conversation

@koydas

@koydas koydas commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

🎯 Goal

Restores the Groq path. Both default models in config/models.yaml have been retired by Groq, so every Groq call currently fails with 404 model_not_found (seen on the review job of #159).

Model Stages Groq shutdown
qwen/qwen3-32b validation, review 2026-07-17
llama-3.3-70b-versatile generation, autofix 2026-08-16

📦 Changes

  • New default model: all four stages now use openai/gpt-oss-120b, Groq's recommended replacement. GROQ_MODEL still overrides every stage.
  • Reasoning effort:
    • gpt-oss is a reasoning model, and its reasoning tokens count toward max_tokens and TPM.
    • New optional key <stage>_reasoning_effort (low | medium | high), validated by loadLLMConfig().
    • It is forwarded by all four entrypoints, including loadConfigFromEnv(), which previously dropped unlisted fields.
    • groq_client sends it only when it is set, because non-reasoning models reject the parameter.
    • Every stage is set to low.
    • New repository variable GROQ_REASONING_EFFORT overrides every stage; off stops sending the parameter (for a non-reasoning GROQ_MODEL).
  • Explicit output caps: validation_max_tokens: 1024, generation_max_tokens: 4096, review_max_tokens: 1024.
  • Auto-fix budget: autofix_max_input_tokens goes from 7,400 to 3,000, so that system (~890, now measured with estimateTokens()) + input + 4,096 output ≈ 7,986 fits the 8K free-tier TPM.
  • Context window: auto_fix_pr now knows the model's 131,072-token context window.
  • ADR-0025 (renumbered: feat(review): ground PR review verdict in executed tool evidence (ADR-0024) #160 took ADR-0024). ADR-0005 is superseded, ADR-0017 amended. Updates to README, AGENTS, docs/code-generation.md and the runbook (rows for 404 model_not_found, 400 reasoning_effort, 413).

🧪 Validation

  • Tests pass: 804/804 after merging main. Lint, the c8 coverage gate on config.mjs and the changelog check pass.
  • Tests added, each failing before the fix:
    • No stage defaults to a retired model.
    • Every Groq stage sets an explicit max_tokens.
    • reasoningEffort is loaded and validated, including the absent-key, global-fallback and invalid-value branches, and the GROQ_REASONING_EFFORT override/off/empty/invalid cases.
    • loadConfigFromEnv forwards it.
    • callGroq sends or omits reasoning_effort; callAnthropic ignores it.
    • End-to-end wiring in all four entrypoints: the Groq request body has model: openai/gpt-oss-120b, max_tokens and reasoning_effort: low.
    • The auto-fix budget is ≤ 8,000 TPM, with the system prompt measured rather than hard-coded.

⚠️ Risks

  • Assumes the Groq free tier (8K TPM), as ADR-0017 already did. On the Developer plan, raise or remove autofix_max_input_tokens.
  • 8K TPM is tight.
    • review (~6.5K prompt tokens + 1,024 output) and generation (no input cap) can hit 413/429 on large PRs or issues.
    • Since feat(review): ground PR review verdict in executed tool evidence (ADR-0024) #160, a review with failing checks also carries up to 2,000 chars of tool-evidence output per failing check; that case is the most likely to exceed 8K.
    • Auto-fix now gets less than half its previous input budget.
  • Unverified model data: the Groq console wasn't reachable from the environment I worked in. Model IDs, dates and limits were cross-checked against search-indexed deprecation pages and third-party reports; please check them against https://console.groq.com/docs/deprecations.
  • Anthropic is still broken: this PR doesn't touch the invalid Anthropic key (401). Groq becomes the only working provider.
  • Not verified in CI before merge: since ADR-0023, pr-review runs main's config, so the review check on this PR will still use the retired model and fail. The new config only takes effect after merge.

🔁 Checklist

  • Clean code
  • No duplication

🧪 How to test

  1. npm test
  2. After merge, push to any open PR. The review job should call Groq with openai/gpt-oss-120b and get past the LLM call.

🤖 Generated with Claude Code

https://claude.ai/code/session_013b1tRLHJqkbkfEZKwonphu

Groq retired qwen/qwen3-32b (validation, review; 2026-07-17) and
llama-3.3-70b-versatile (generation, autofix; 2026-08-16), so every
Groq call failed with 404 model_not_found.

- All stages default to openai/gpt-oss-120b (GROQ_MODEL still
  overrides).
- gpt-oss is a reasoning model: new optional <stage>_reasoning_effort
  key (low|medium|high), validated by loadLLMConfig(), forwarded by all
  four entrypoints (incl. loadConfigFromEnv) and sent by groq_client
  only when set, since non-reasoning models reject it. Set to low.
- autofix_max_input_tokens 7400 -> 3400 to fit the 8K free-tier TPM
  (system + input + 4096 output ~= 7956); a test asserts the sum.
- auto_fix_pr knows the 131,072-token context window.
- ADR-0024; README, AGENTS, code-generation and runbook updated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GzuUtVET9ZuK7cbx2LRUZM
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

koydas commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner Author

Code review — fix(groq): replace retired models with openai/gpt-oss-120b

Update 2 — merge conflict resolved in 41fa5f8. The PR was dirty against main after #160 merged:

Local validation on 41fa5f8: node --test 804/804, npm run lint OK, check_changelog OK.

Update 1 — fixes pushed in b95f719. Every item below is struck through and has a ✔️ note saying what was done. Items not addressed are marked ⏭️ with the reason. Local validation on b95f719:

  • node --test: 735/735 (+12 tests)
  • npm run lint: OK
  • c8 coverage on config.mjs: 95.6% lines / 90.8% branches
  • check_changelog: OK

New finding while fixing: auto-fix-system.md now measures ~889 tokens, not ~460. The 3,400 budget therefore totaled ~8,385 tokens, over the 8K limit. It is lowered to 3,000 (≈ 7,986).

Verdict: approve once the pre-merge checks below are done ✔️ Approvable. No blockers in the code, no merge conflict. The review check stays red until merge (ADR-0023: it runs main's config).

Original validation on 46fbf62: 723/723. CI: test ✅, changelog ✅, review ❌ (expected).


✅ What's good

  • Scoped fix. It handles the incompatibilities that come with a reasoning model: reasoning_effort, reasoning tokens counted in max_tokens/TPM, the 131k context window. It is not just a find/replace of the model ID.
  • reasoning_effort is opt-in in callGroq. It is sent only when set. It is an explicit per-stage key rather than inferred from the model name, and ADR-0025 records why.
  • Validation fits the pattern. loadLLMConfig() validates it the same way as temperature/max_tokens.
  • Silent drop fixed. loadConfigFromEnv() dropped unlisted fields; it now forwards reasoningEffort, with a test.
  • Wiring tested end to end. auto_fix_pr and pr_review are tested at the HTTP boundary.
  • Budget guard. The test that keeps the 8K TPM budget turns ADR-0017's comment math into an invariant. Once the estimate was made real, this test caught the 8,385-token overrun.
  • Honest risks section.
  • Guardrails respected. Edits are additive, nothing is converted between ESM and CJS, no new dependencies, no test count goes down.

🚫 Blockers

None.

⚠️ Warnings

  1. validation, generation and review have no max_tokens. With a reasoning model, if Groq counts the default max_completion_tokens in its per-request TPM check, every call can get a 413. Set an explicit <stage>_max_tokens.
    ✔️ Explicit values in config/models.yaml:

    • validation_max_tokens: 1024
    • generation_max_tokens: 4096
    • review_max_tokens: 1024

    Review is then about 6.5K prompt + 1K output, under 8K. If a review is cut off before its verdict line, it fails closed to REQUEST_CHANGES (regex at pr_review.mjs:231). This is documented in ADR-0025. The new test every Groq stage sets an explicit max_tokens checks it, and the wiring tests assert max_tokens in the Groq request body.

  2. The GROQ_MODEL override is fragile now. A non-reasoning model returns a 400, and the YAML is read from main.
    ✔️ New repository variable GROQ_REASONING_EFFORT:

    • low, medium or high overrides every stage.
    • off stops sending the parameter.
    • An empty value falls back to the YAML.

    It is wired into all 4 workflows and validated: an invalid value throws. Docs: a runbook row for 400 reasoning_effort, plus the env matrix and Quick Start in code-generation.md. Tests cover override, off, empty value, invalid value, and Anthropic ignoring it.

  3. Model facts not yet checked. Check the console before merge.
    ✔️ Cross-checked through search, since console.groq.com is still blocked by this environment's egress proxy:

    • Shutdown dates match Groq's deprecations page as indexed: qwen/qwen3-32b 07/17/26, llama-3.3-70b-versatile 08/16/26.
    • The replacement openai/gpt-oss-120b is confirmed.
    • The free-tier limits (8K TPM / 30 RPM / 1K RPD / 200K TPD) match several independent sources.

    The ADR-0025 caveat is updated with this. A direct read of the console is still advised if a 413/429 pattern appears.

  4. The 8K TPM is tight for review (~6.5K prompt, no input cap).
    ✔️ Partial, and my original claim needs a correction: the review diff is already capped at 12,000 chars (filterDiff, pr_review.mjs:200). Only the PR body and manifests are uncapped. The output cap from W1 bounds the request. ⏭️ I did not add a review_max_input_tokens key: that's a feature, outside this PR's scope. The runbook now names the levers. Since feat(review): ground PR review verdict in executed tool evidence (ADR-0024) #160, tool evidence also counts here (see Update 2).

  5. The ADR chain isn't updated. ADR-0005 and ADR-0017 are still Accepted.
    ✔️ ADR-0005 → Superseded by ADR-0025. ADR-0017 → Accepted — amended by ADR-0025, the same convention as ADR-0010/0022. ADR-0025 now has a Decision item 4 (explicit max_tokens), the corrected budget (3,000, system prompt ~890), and the GROQ_REASONING_EFFORT override.

🔹 Nits

  • AGENTS.md:25 still says "Groq models cap at 32 768 tokens". ✔️ Now says 131,072 for gpt-oss-120b, and that the per-request 8K TPM is the binding limit.
  • The retired-model test pins assert.equal(model, 'openai/gpt-oss-120b'). ✔️ Removed. Only the RETIRED_GROQ_MODELS check remains.
  • SYSTEM_PROMPT_TOKENS_EST = 460 is hard-coded. ✔️ Now estimateTokens(loadPrompt('auto-fix-system')). This exposed the overrun: the real figure is ~889, so autofix_max_input_tokens goes 3,400 → 3,000. Updated in YAML, AGENTS, code-generation.md, runbook, ADR-0025, ADR-0017 and the CHANGELOG. ADR-0017's old 7,400 budget was in fact over 12K as well (≈ 12,385).
  • The global reasoning_effort fallback is untested. ✔️ Test added: falls back to the global reasoning_effort key when the stage key is absent.
  • MODEL_CONTEXT_WINDOW keeps the retired IDs. ✔️ // retired by Groq <date> (ADR-0025) comments added.
  • Pre-existing, out of scope: callLLM falls back to Anthropic with the Groq model. ⏭️ Not touched; it's a separate change from this PR. Worth an issue.

📊 Coverage

Area Status Notes
Test coverage ✅ ❌ Missing: validate_issue/generate_issue_change forwarding, the global fallback key, Anthropic ignoring the param. ✔️ New reasoning_effort_wiring.test.mjs (4 E2E tests at the HTTP boundary: model, max_tokens, reasoning_effort, off, override). +7 tests in config.test.mjs, +1 in anthropic_client.test.mjs. 723 → 735 → 804 after merging main.
Documentation coverage ✅ 🟡 Missing: AGENTS.md:25, runbook row for 400, <stage>_reasoning_effort key in the config table. ✔️ All three done. New Per-Stage Model Keys table in code-generation.md (<stage>, _temperature, _max_tokens, _reasoning_effort). GROQ_REASONING_EFFORT added to the env matrix.
ADR created ✅ ADR-0024 ADR-0025 (renumbered, see Update 2) is indexed and has its CHANGELOG entry. 🟡 ADR-0005/0017 not marked superseded/amended. ✔️ Done (W5).
Architecture diagram ✅ N/A The Mermaid diagram in the README shows the pipeline flow, which this PR doesn't change.
Changelog gate ✅ The entry is updated with the new changes. check_changelog OK locally.
Observability ✅ No new stage. ⏭️ The optional reasoningEffort in *.llm_request meta is not added: it was optional and outside scope.

Suggested before merge

  1. Check the Groq facts (Warning 3). ✔️ Cross-checked (see W3).
  2. Set max_tokens on the 3 uncapped stages. ✔️ Done (W1).
  3. Update the ADR-0005/0017 statuses and AGENTS.md:25. ✔️ Done.
  4. Resolve the merge conflict with main (feat(review): ground PR review verdict in executed tool evidence (ADR-0024) #160). ✔️ Done in 41fa5f8 (Update 2).
  5. After merge: push to an open PR and confirm the review job gets past the LLM call (no 404/413/400). Still to do; it can only be checked after merge (ADR-0023).

Generated by Claude Code

claude and others added 2 commits October 1, 2026 04:22
…T override

Addresses review on #161:
- Explicit <stage>_max_tokens for validation (1024), generation (4096), review (1024).
- autofix_max_input_tokens 3400 -> 3000: auto-fix-system.md measures ~890 tokens,
  not ~460, so the previous budget totaled ~8,385 > 8K TPM. Budget test now
  measures the prompt with estimateTokens() instead of a hard-coded constant.
- GROQ_REASONING_EFFORT repo variable overrides every stage; `off` drops the
  parameter for non-reasoning GROQ_MODEL overrides. Wired into all 4 workflows.
- ADR-0005 superseded, ADR-0017 amended; ADR-0024, AGENTS (stale 32K context
  line), code-generation (per-stage keys table, env matrix), runbook (400 row).
- Tests: validate_issue/generate_issue_change wiring E2E, global fallback key,
  env override/off/invalid, Anthropic ignores reasoningEffort, explicit
  max_tokens on every stage. Retired-model test no longer pins the exact ID.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RQ2EvwfnVxY9PXffXtc3S4
…to 0025

#160 took ADR-0024 (tool evidence for PR review). The Groq model ADR becomes
ADR-0025: file renamed, index, ADR-0005/0017 status links and every Groq
reference updated. pr_review.test.mjs conflict resolved by keeping both sides
(44 tests = 36 base + 1 here + 7 from #160).

ADR-0025 and the runbook 413 row now account for the tool-evidence block
(up to 2,000 chars per failing check) added to the review prompt by #160,
which reduces the 8K TPM headroom of the review stage.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013b1tRLHJqkbkfEZKwonphu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants