fix(ci): unbreak shell-inline-check on main - #1487
Conversation
… inline script The awk check used `for (i in lines)` which has undefined iteration order, so continuation lines were often checked before the bash-delegate line, producing false positives on every multi-line `home.activation` block. Iterate 1..n so the bash-call gate flips before its continuations are seen. Also extract the dolt startScript to an external start.sh referenced via pkgs.replaceVars, matching the pattern used by obsidian/docker services.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExtracted the inline dolt service startup logic from Nix configuration into an external shell script, delegating control flow (directory creation, migration, symlinking, and SQL server execution) to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Mesa DescriptionTL;DRFixes the failing CI What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Dolt service by moving its start script to an external file and utilizing pkgs.replaceVars for variable substitution. Additionally, it improves the reliability of the check-nix-inline-scripts.sh script by ensuring ordered iteration over lines in AWK. Feedback was provided regarding the use of builtins.readFile on a derivation, which introduces an Import From Derivation (IFD) that could impact evaluation performance; using pkgs.substituteAll is recommended as a more efficient alternative.
| startScript = pkgs.writeShellScript "dotfiles-dolt-start.sh" ( | ||
| builtins.readFile ( | ||
| pkgs.replaceVars ./start.sh { | ||
| inherit beadsDir; | ||
| inherit (pkgs) dolt; | ||
| } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
This implementation uses builtins.readFile on a derivation produced by pkgs.replaceVars, which is an Import From Derivation (IFD). IFD can cause performance issues during Nix evaluation because it forces a build to happen before evaluation can continue.
Since this follows an existing pattern in the repository, it may be acceptable, but a more efficient alternative would be to use pkgs.substituteAll to create the script derivation directly, avoiding the need to read the file back into Nix memory.
The coverage_spec.sh diffs `git ls-files '*.sh'` against the covered_scripts allowlist and fails shell-test if either side has an entry the other doesn't. Register the new script and add a basic spec covering shebang, strict mode, placeholder tokens, migration behavior, and sql-server flags.
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
home-manager/services/dolt/default.nix (1)
27-27:⚠️ Potential issue | 🔴 Critical
ProgramArgumentsexpects strings, not derivations — add string interpolation.CI reports type validation failure:
definition for option
...launchd.agents.dolt.config.ProgramArguments...is not of typestring. Definition value:<derivation dotfiles-dolt-start.sh>.
pkgs.writeShellScriptreturns a derivation, butProgramArgumentslist items must be strings. All similar usages in the codebase (cliproxyapi, tmux-session-logger, code-syncer, and others) coerce script variables with string interpolation. Apply the same pattern:- ProgramArguments = [ startScript ]; + ProgramArguments = [ "${startScript}" ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/services/dolt/default.nix` at line 27, ProgramArguments currently contains a derivation (startScript produced by pkgs.writeShellScript) but must be a list of strings; update the launchd config to interpolate the script path into a string (e.g. use "${startScript}" style interpolation) so ProgramArguments contains string values. Locate the ProgramArguments entry and replace the raw startScript symbol with a string-interpolated version (same pattern used for cliproxyapi, tmux-session-logger, code-syncer) ensuring startScript remains created via pkgs.writeShellScript but is referenced as a string in ProgramArguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/dolt_start_spec.sh`:
- Around line 18-21: The sed placeholder regex in the test "passes bash syntax
check after stripping placeholders" is too narrow (currently
's|@[a-z_]*@|/usr|g') and misses placeholders with uppercase letters or digits;
update the sed expression used in the bash -c invocation to a broader pattern
such as 's|@[A-Za-z_][A-Za-z0-9_]*@|/usr|g' so `@beadsDir`@ and similar
placeholders are stripped before running bash -n.
---
Outside diff comments:
In `@home-manager/services/dolt/default.nix`:
- Line 27: ProgramArguments currently contains a derivation (startScript
produced by pkgs.writeShellScript) but must be a list of strings; update the
launchd config to interpolate the script path into a string (e.g. use
"${startScript}" style interpolation) so ProgramArguments contains string
values. Locate the ProgramArguments entry and replace the raw startScript symbol
with a string-interpolated version (same pattern used for cliproxyapi,
tmux-session-logger, code-syncer) ensuring startScript remains created via
pkgs.writeShellScript but is referenced as a string in ProgramArguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edcbeb7e-3b82-4714-a8ab-34f5d50aafab
📒 Files selected for processing (5)
home-manager/services/dolt/default.nixhome-manager/services/dolt/start.shscripts/check-nix-inline-scripts.shspec/coverage_spec.shspec/dolt_start_spec.sh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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/dolt_start_spec.sh">
<violation number="1" location="spec/dolt_start_spec.sh:19">
P2: The placeholder-stripping regex misses camelCase placeholders (e.g. `@beadsDir@`), so this syntax-check test is weaker than intended.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
nix-darwin on galactica was failing with: definition for option `launchd.agents.dolt.config.ProgramArguments' is not of type `string'. Definition value: <derivation dotfiles-dolt-start.sh> `pkgs.writeShellScript` returns a derivation; ProgramArguments expects strings, so interpolate to coerce to the store path, matching the pattern used by cliproxyapi/tmux-session-logger/code-syncer. Also widen the placeholder-stripping regex in dolt_start_spec.sh so it catches camelCase tokens like @beadsDir@ (previously only lowercase was stripped, making the bash -n smoke test weaker than intended).
fishtape wraps the remaining arguments of @test with `test`, so @test "unknown session returns failure" test \$unknown_status -ne 0 expanded to `test test 1 -ne 0` (5 args) which fish's test rejects, causing shell-test to fail since PR #1479 introduced this spec. Drop the literal `test` so fishtape produces `test 1 -ne 0`.
2 participants
Summary
shell-inline-checkhas been failing onmainsince the check was introduced. Two bugs, one fix each:scripts/check-nix-inline-scripts.shwalkedhome.activationblock lines withfor (i in lines), whose order is indeterminate. Continuation lines ("arg" \) were often inspected before thebash-delegate line that's supposed to setsaw_bash_call = 1, so every multi-line activation block innamed-hosts/kyber,named-hosts/matic, andhome-manager/modules/tailscaletripped the "unknown-line" branch. Switched to an orderedfor (i = 1; i <= n; i++)using the count returned bysplit.home-manager/services/dolt/default.nixhad apkgs.writeShellScriptwith a large inline string, which the first check rejects. Extracted it tohome-manager/services/dolt/start.shand wired it up throughpkgs.replaceVars, matching the pattern already used for obsidian/docker services. The${beadsDir}and${pkgs.dolt}interpolations become@beadsDir@/@dolt@placeholders.Test plan
bash scripts/check-nix-inline-scripts.sh→No inline shell or Python scripts in Nix filesbash -non both touched shell scriptsshell-inline-checkgreen on this PRhttps://claude.ai/code/session_017g5bYHcPU5fBNebzkrX3td
Summary by cubic
Fixes the failing CI
shell-inline-checkonmainand a fishtape error. Orders AWK line processing, extracts the Dolt start script, adds tests/coverage, and stringifies the path forlaunchdProgramArguments.home.activationblocks.home-manager/services/dolt/start.shviapkgs.replaceVars; addspec/dolt_start_spec.shand register it inspec/coverage_spec.sh(widen placeholder-stripping regex forbash -n).launchdProgramArgumentsby interpolating"${startScript}"; remove a straytestin fishtape assertions to prevent double-wrapping inspec/fish/__tmux_bootstrap_default_session_test.fish.Written for commit dc7a8c0. Summary will update on new commits.