fix(nix): unbreak t3code build and shell-test on main - #2363
Conversation
Entire-Checkpoint: 7e7a1e43230b
…odels Entire-Checkpoint: f09646b18b10
…ec block Entire-Checkpoint: 916a934c741d
Entire-Checkpoint: 532bf810626b
Entire-Checkpoint: 6b95ab79e8cb
Entire-Checkpoint: c88fab3612ed
Entire-Checkpoint: 30a66d0885de
Entire-Checkpoint: b5bf7137ed61
…pport-335ac0 # Conflicts: # spec/llm_update_spec.sh
Entire-Checkpoint: a3af5408972b
Entire-Checkpoint: ed227c6aedcb
|
Warning Indent Zero is shutting down on August 7th. Please migrate over to Indent 2.0 to continue getting PR reviews.
|
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| # it needs repointing at the repinned build rather than its own fix. | ||
| // prev.lib.optionalAttrs (prev.llm-agents ? t3code-desktop) { | ||
| t3code-desktop = prev.llm-agents.t3code-desktop.overrideAttrs (_: { | ||
| paths = [ t3code.desktop ]; |
There was a problem hiding this comment.
Latent: paths reset loses future symlinkJoin inputs.
This sets paths = [ t3code.desktop ], fully replacing whatever upstream's t3code-desktop symlinkJoin had. Today that's fine — upstream joins a single path — but if a future llm-agents.nix release adds a second entry (icon set, launcher wrapper, shared assets), it will be silently stripped on x86_64-linux only, while other systems keep the full join.
Consider preserving the tail with a map, e.g.:
t3code-desktop = prev.llm-agents.t3code-desktop.overrideAttrs (old: {
paths = map (p: if p == prev.llm-agents.t3code.desktop then t3code.desktop else p) old.paths;
});or at least asserting old.paths has length 1 so a future upstream expansion trips the build instead of silently narrowing it.
| elif base(model) in [base(entry) for entry in chain]: | ||
| index = [base(entry) for entry in chain].index(base(model)) | ||
| else: | ||
| continue |
There was a problem hiding this comment.
Latent: silent pass on parse skew.
Any role whose model isn't matched by exact or base selector hits continue, so a formatter change that (a) drops a role from roles, (b) breaks the 4/6-space chain indentation, or (c) inlines a comment after the value will cause the role to be skipped and the test to pass vacuously. Since the two re.search().group(1) calls only hard-fail on the top-level modelRoles: / fallbackChains: headers, most drift goes undetected.
Suggest asserting the parsed structure is non-empty and treating an empty resolved chain as a failure, e.g.:
assert roles, "no modelRoles parsed"
assert chains, "no fallbackChains parsed"
...
chain = chains.get(role) or chains.get("default") or []
if not chain:
stranded.append(f"{role} has no resolved fallback chain")
continue
|
|
||
| It 'uses OMP native retry fallback chains instead of an extension' | ||
| When run bash -c "[ ! -e config/omp/fallback.ts ] && [ ! -e config/omp/fallback.json ] && yq -e '.retry.modelFallback == true and .retry.fallbackRevertPolicy == \"cooldown-expiry\" and .retry.fallbackChains.default == [\"openai-codex/gpt-5.6-luna\",\"openai-codex/gpt-5.3-codex-spark\",\"openai-codex/gpt-5.6-sol\"]' config/omp/config.yml >/dev/null" | ||
| When run bash -c "[ ! -e config/omp/fallback.ts ] && [ ! -e config/omp/fallback.json ] && grep -q 'modelFallback: true' config/omp/config.yml && grep -q 'fallbackRevertPolicy: \"cooldown-expiry\"' config/omp/config.yml" |
There was a problem hiding this comment.
Nit: coverage regression on the default chain.
The previous assertion pinned .retry.fallbackChains.default to the three-entry expected list, which is exactly what would have caught openai-codex/gpt-5.3-codex-spark sitting stale in this file. The rewrite only checks the two flag lines, so this test would no longer flag the class of bug this PR fixes (removed placeholders / unresolvable model IDs in the default chain).
Since the reason for dropping yq was dialect divergence, one option is to reuse the new Python helper: either extend omp_fallback_chain_tails_ok to also assert chains["default"] equals the expected list, or add a small sibling helper and invoke it here with When call.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/llm_update_spec.sh">
<violation number="1" location="spec/llm_update_spec.sh:39">
P2: When a role's model fails to match by exact or base selector, the loop just `continue`s without recording a failure. If a future formatting change drops a role from `roles`, breaks the chain indentation, or inlines a comment after a value, the role gets silently skipped and this test passes vacuously instead of flagging the parse regression. Add assertions that `roles`/`chains` are non-empty and treat an empty resolved chain as a failure rather than silently continuing.</violation>
<violation number="2" location="spec/llm_update_spec.sh:231">
P3: The default OMP fallback chain is no longer pinned: the old yq check asserted `fallbackChains.default == [openai-codex/gpt-5.6-luna, ..., gpt-5.6-sol]`, but the replacement only greps for `modelFallback: true` and `fallbackRevertPolicy`. A regression in the default chain's entries (or an empty `default:` block) now passes both tests, since `omp_fallback_chain_tails_ok` skips roles that resolve to nothing. Since issue #2360 was specifically about the contents of that chain, restore a concrete content check for the default chain instead of dropping it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| index = chain.index(model) | ||
| elif base(model) in [base(entry) for entry in chain]: | ||
| index = [base(entry) for entry in chain].index(base(model)) | ||
| else: |
There was a problem hiding this comment.
P2: When a role's model fails to match by exact or base selector, the loop just continues without recording a failure. If a future formatting change drops a role from roles, breaks the chain indentation, or inlines a comment after a value, the role gets silently skipped and this test passes vacuously instead of flagging the parse regression. Add assertions that roles/chains are non-empty and treat an empty resolved chain as a failure rather than silently continuing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/llm_update_spec.sh, line 39:
<comment>When a role's model fails to match by exact or base selector, the loop just `continue`s without recording a failure. If a future formatting change drops a role from `roles`, breaks the chain indentation, or inlines a comment after a value, the role gets silently skipped and this test passes vacuously instead of flagging the parse regression. Add assertions that `roles`/`chains` are non-empty and treat an empty resolved chain as a failure rather than silently continuing.</comment>
<file context>
@@ -4,6 +4,49 @@
+ index = chain.index(model)
+ elif base(model) in [base(entry) for entry in chain]:
+ index = [base(entry) for entry in chain].index(base(model))
+ else:
+ continue
+ if index == len(chain) - 1:
</file context>
|
|
||
| It 'uses OMP native retry fallback chains instead of an extension' | ||
| When run bash -c "[ ! -e config/omp/fallback.ts ] && [ ! -e config/omp/fallback.json ] && yq -e '.retry.modelFallback == true and .retry.fallbackRevertPolicy == \"cooldown-expiry\" and .retry.fallbackChains.default == [\"openai-codex/gpt-5.6-luna\",\"openai-codex/gpt-5.3-codex-spark\",\"openai-codex/gpt-5.6-sol\"]' config/omp/config.yml >/dev/null" | ||
| When run bash -c "[ ! -e config/omp/fallback.ts ] && [ ! -e config/omp/fallback.json ] && grep -q 'modelFallback: true' config/omp/config.yml && grep -q 'fallbackRevertPolicy: \"cooldown-expiry\"' config/omp/config.yml" |
There was a problem hiding this comment.
P3: The default OMP fallback chain is no longer pinned: the old yq check asserted fallbackChains.default == [openai-codex/gpt-5.6-luna, ..., gpt-5.6-sol], but the replacement only greps for modelFallback: true and fallbackRevertPolicy. A regression in the default chain's entries (or an empty default: block) now passes both tests, since omp_fallback_chain_tails_ok skips roles that resolve to nothing. Since issue #2360 was specifically about the contents of that chain, restore a concrete content check for the default chain instead of dropping it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/llm_update_spec.sh, line 231:
<comment>The default OMP fallback chain is no longer pinned: the old yq check asserted `fallbackChains.default == [openai-codex/gpt-5.6-luna, ..., gpt-5.6-sol]`, but the replacement only greps for `modelFallback: true` and `fallbackRevertPolicy`. A regression in the default chain's entries (or an empty `default:` block) now passes both tests, since `omp_fallback_chain_tails_ok` skips roles that resolve to nothing. Since issue #2360 was specifically about the contents of that chain, restore a concrete content check for the default chain instead of dropping it.</comment>
<file context>
@@ -180,12 +223,12 @@ End
It 'uses OMP native retry fallback chains instead of an extension'
-When run bash -c "[ ! -e config/omp/fallback.ts ] && [ ! -e config/omp/fallback.json ] && yq -e '.retry.modelFallback == true and .retry.fallbackRevertPolicy == \"cooldown-expiry\" and .retry.fallbackChains.default == [\"openai-codex/gpt-5.6-luna\",\"openai-codex/gpt-5.3-codex-spark\",\"openai-codex/gpt-5.6-sol\"]' config/omp/config.yml >/dev/null"
+When run bash -c "[ ! -e config/omp/fallback.ts ] && [ ! -e config/omp/fallback.json ] && grep -q 'modelFallback: true' config/omp/config.yml && grep -q 'fallbackRevertPolicy: \"cooldown-expiry\"' config/omp/config.yml"
The status should be success
End
</file context>
1 participant
Three fixes for red builds on
main.1. t3code build failure (
overlays/default.nix)make buildfails on every x86_64-linux host:Upstream pins a single
pnpmDepshash for t3code 0.0.33, butfetchPnpmDepsresolves platform-specific optional packages, so it only reproduces on the system upstream generated it from. Repinned for x86_64-linux only — other systems keep the upstream hash and cannot regress. Follows the existinggrok/bernsteinworkaround pattern.t3code-desktopneeded separate handling: it is asymlinkJoinover t3code'sdesktopoutput with nopnpmDepsof its own, so overridingt3codealone left it pulling the broken build. Itspathsare repointed at the repinned derivation.Verified by eval against the
maticconfig —t3code.pnpmDeps.outputHashis the corrected hash, andt3code-desktop.pathsresolves to a new store path rather than the original.Drop this override once upstream repins. A
nix flake update llm-agentswill not help: their last t3code commit predates our pin.2. shell-test failure (
spec/llm_update_spec.sh)#2358 merged with shell-test red — two specs I added used
yqwith jq-syntax filters.yqis the kislyuk Python build locally and the mikefarah Go build in CI, and no filter is valid in both:Rewritten to parse the generated config with python3 regexes — no YAML library, no dialect assumption.
python3is already used by three existing specs;yqwas used by none before mine. Also adds a guard that every Pi fallback model resolves againstconfig/pi/models.json, since an unresolvable entry silently cools the whole chain down at runtime.3. Dead placeholder (
config/omp/config.tpl.yml)#2360 removed
gpt-codex-sparkfrom the canonicalmodels.json, but the OMPdefaultfallback chain still referenced__GPT_CODEX_SPARK__, so the nextllm-update.shrun rendered the literal placeholder into the config. Repointed to__GPT_CODEX__.Testing
nix fmtclean.Summary by cubic
Fixes red builds on main by repinning
t3codepnpmDepson x86_64-linux, repointingt3code-desktop, replacing a removed OMP model in the default chain, and making OMP shell tests independent ofyqdialects. Before: x86_64-linux builds failed on a fixed-output hash mismatch, CI shell tests errored onyq, and the template rendered a dead placeholder. After: x86_64-linux builds pass (other platforms unchanged), tests parse withpython3, and configs reference__GPT_CODEX__/gpt-5.3-codex.t3codepnpmDeps.outputHashforx86_64-linuxand repointst3code-desktop.pathsto the repinneddesktopoutput; other systems keep the upstream hash. Remove this override once upstream repins (anix flake update llm-agentswill not fix this).spec/llm_update_spec.shto parse OMP config withpython3regexes instead ofyq, avoiding kislyuk vs mikefarah dialect conflicts; adds a guard that every Pi fallback model resolves againstconfig/pi/models.json.__GPT_CODEX_SPARK__/gpt-5.3-codex-sparkwith__GPT_CODEX__/gpt-5.3-codexin the OMP default fallback chain to prevent rendering a dead placeholder.Written for commit 6b03679. Summary will update on new commits.