fix: improve docker ci cache reuse - #1597
Closed
shunkakinoki wants to merge 4 commits into
Closed
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughMigrates Nix GitHub token handling from embedding credentials in the Docker image to BuildKit secrets, adds a pre-install Nix flake cache warmup, stages flake/install files into the image for local execution, hardens Makefile git metadata handling, and adds tests and coverage entries for the new warmup script. Also adds a DarkModeToggle widget. ChangesNix Token & Cache Warmup
Noctalia Bar Widget
Sequence DiagramsequenceDiagram
actor Builder as Docker Builder
participant Docker as Image Build
participant Secret as BuildKit Secret
participant Daemon as Nix Daemon
participant Make as Make (nix-cache-warmup)
participant Warmup as nix-cache-warmup.sh
participant Install as install.sh
participant Nix as nix CLI
Builder->>Docker: COPY flake + scripts -> /tmp/dotfiles-cache/
Builder->>Secret: Mount github_token as secret -> GITHUB_TOKEN_FILE
Docker->>Daemon: Start nix-daemon
Docker->>Make: make -C /tmp/dotfiles-cache nix-cache-warmup (GITHUB_TOKEN_FILE)
Make->>Warmup: invoke scripts/nix-cache-warmup.sh . $(NIX_FLAGS)
Warmup->>Secret: read token from GITHUB_TOKEN_FILE (if present)
Warmup->>Warmup: set NIX_CONFIG access-tokens entry
Warmup->>Nix: run `nix flake metadata` --no-write-lock-file
Nix-->>Warmup: returns metadata (cache warmed)
Docker->>Install: sh /tmp/dotfiles-cache/install.sh (COMMIT_SHA, GITHUB_PR, GITHUB_TOKEN_FILE)
Install->>Secret: read token from GITHUB_TOKEN_FILE (if present)
Install->>Install: export NIX_CONFIG with access-tokens
Install->>Nix: perform Nix operations for install
Nix-->>Install: installation complete
Install-->>Docker: dotfiles installed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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;DRImproved Docker CI cache reuse and build performance by centralizing GitHub token handling, adding a Nix cache warming script, and optimizing the Docker build context. What changed?
Description generated by Mesa. Update settings |
shunkakinoki
force-pushed
the
fix/docker-cache-reuse
branch
from
April 27, 2026 14:08
f434539 to
f2b39cd
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves the security and efficiency of the build process by utilizing Docker secrets for GitHub tokens instead of build arguments and adding a Nix cache warmup step to the Makefile. Additionally, the install.sh script now dynamically configures Nix access tokens from environment variables or secret files. A review comment suggests using a space instead of a newline when appending to the NIX_CONFIG environment variable to ensure better compatibility and readability.
Comment on lines
+50
to
+51
| NIX_CONFIG="${NIX_CONFIG} | ||
| access-tokens = github.com=$NIX_GITHUB_TOKEN" |
Contributor
There was a problem hiding this comment.
When appending to NIX_CONFIG, it is safer to ensure there is a space or newline between existing configuration and the new access-tokens entry. While the literal newline works in most POSIX-compliant shells, using a space as a separator is more conventional for environment variable based configuration if it's intended to be a single line, or ensuring the newline is robustly handled.
Suggested change
| NIX_CONFIG="${NIX_CONFIG} | |
| access-tokens = github.com=$NIX_GITHUB_TOKEN" | |
| NIX_CONFIG="${NIX_CONFIG} access-tokens = github.com=$NIX_GITHUB_TOKEN" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
spec/nix_cache_warmup_spec.sh (1)
19-69: These checks only prove the strings exist, not that the script still behaves correctly.Most of this spec is
grep-based, so it will still pass ifscripts/nix-cache-warmup.shstops exportingNIX_CONFIG, mishandlesGITHUB_TOKEN_FILE, or exits non-zero whennixis unavailable, as long as the same literals remain in the file. Since this script is now in the Docker build path, I'd add at least one execution-style spec with a stubnixcommand and controlled env.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/nix_cache_warmup_spec.sh` around lines 19 - 69, Add an execution-style test to replace the grep-only checks: run the script ($SCRIPT) in a temporary PATH where a stub "nix" binary is provided (or absent) and with controlled env vars (GITHUB_TOKEN and/or GITHUB_TOKEN_FILE) to assert real behavior — e.g., that NIX_CONFIG is exported/contains "access-tokens", that "unset nix_github_token" occurs, that the script exits 0 when nix is missing and prints "skipping cache warmup", and that "nix flake metadata" is invoked with "--no-write-lock-file"; implement this in spec/nix_cache_warmup_spec.sh by adding a new example which sets PATH to a temp dir with a stub `nix` script (or leaves it out) and then executes "$SCRIPT" checking stdout/stderr and exit status instead of only grepping literals like 'GITHUB_TOKEN', 'NIX_CONFIG', 'nix flake metadata', '--no-write-lock-file', and 'repo_dir=${1:-.}'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 63-73: The Dockerfile currently defaults ARG COMMIT_SHA=main and
uses curl to fetch install.sh from GitHub, causing local builds to warm cache
from the checkout but then install a different revision; replace the remote curl
step with executing the checked-out install.sh from the build context (or ensure
the local copy is copied into the image) so branch-local changes are exercised.
Specifically, stop relying on COMMIT_SHA fallback to main in the RUN --mount=...
line and instead COPY the repository's install.sh into the image during build
and run that local file as the unprivileged user (referencing the existing ARG
COMMIT_SHA and the RUN --mount=type=secret,id=github_token,mode=0444 block and
the install invocation) so the image executes the exact checked-out script
rather than fetching main from GitHub.
In `@Makefile`:
- Around line 435-443: The nix-cache-warmup target lists prerequisites
(nix-connect, nix-trust) that are executed before the recipe, which bypasses the
in-recipe `command -v nix` guard on systems without Nix; to fix this, remove
nix-connect and nix-trust from the target prerequisites and instead invoke them
inside the recipe only after the `command -v nix` check (use $(MAKE) to run
nix-connect and nix-trust from within the if-branch), then proceed to run
./scripts/nix-cache-warmup.sh and echo success; keep the fallback echo in the
else branch unchanged so the skip message is shown when Nix is absent.
- Line 154: The install target currently lists "nix-cache-warmup" as a sibling
prerequisite so GNU Make may run it in parallel; change the dependency graph so
nix-cache-warmup runs before the expensive steps by making "nix-build" and
"nix-switch" depend on "nix-cache-warmup" (e.g., add a rule "nix-build:
nix-cache-warmup" and "nix-switch: nix-cache-warmup" or otherwise make them list
nix-cache-warmup as a prerequisite) instead of only listing all targets on the
"install" line.
---
Nitpick comments:
In `@spec/nix_cache_warmup_spec.sh`:
- Around line 19-69: Add an execution-style test to replace the grep-only
checks: run the script ($SCRIPT) in a temporary PATH where a stub "nix" binary
is provided (or absent) and with controlled env vars (GITHUB_TOKEN and/or
GITHUB_TOKEN_FILE) to assert real behavior — e.g., that NIX_CONFIG is
exported/contains "access-tokens", that "unset nix_github_token" occurs, that
the script exits 0 when nix is missing and prints "skipping cache warmup", and
that "nix flake metadata" is invoked with "--no-write-lock-file"; implement this
in spec/nix_cache_warmup_spec.sh by adding a new example which sets PATH to a
temp dir with a stub `nix` script (or leaves it out) and then executes "$SCRIPT"
checking stdout/stderr and exit status instead of only grepping literals like
'GITHUB_TOKEN', 'NIX_CONFIG', 'nix flake metadata', '--no-write-lock-file', and
'repo_dir=${1:-.}'.
🪄 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: 86834b37-98d3-41c4-bf1e-4209e2a98e92
📒 Files selected for processing (8)
.dockerignore.github/workflows/docker.ymlDockerfileMakefileinstall.shscripts/nix-cache-warmup.shspec/coverage_spec.shspec/nix_cache_warmup_spec.sh
✅ Files skipped from review due to trivial changes (1)
- scripts/nix-cache-warmup.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .dockerignore
- install.sh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 52 files (changes from recent commits).
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="Makefile">
<violation number="1" location="Makefile:302">
P2: `make update` no longer runs `nix-update`, so it no longer updates Nix despite the target description saying it does.</violation>
</file>
<file name="install.sh">
<violation number="1" location="install.sh:50">
P1: Preserve the newline separator here; a space can make the existing NIX_CONFIG entry and `access-tokens` collapse into one invalid line.</violation>
</file>
<file name="config/hermes/hydrate.sh">
<violation number="1" location="config/hermes/hydrate.sh:73">
P2: Escape secret values before using them in `sed` replacements; unescaped `&`, `|`, or `\` can break hydration and generate invalid config.</violation>
</file>
<file name=".github/workflows/docker.yml">
<violation number="1">
P1: `push: true` on pull requests causes PR builds to publish images/tags instead of staying build-only. Restore event/branch gating for push.</violation>
</file>
<file name="config/hermes/config.tpl.yaml">
<violation number="1" location="config/hermes/config.tpl.yaml:265">
P1: Secret redaction is turned off by default, which can expose credentials in logs and tool output.</violation>
</file>
<file name="home-manager/services/hermes/default.nix">
<violation number="1" location="home-manager/services/hermes/default.nix:37">
P2: Avoid writing Hermes gateway logs to `/tmp`; use the private `${homeDir}/.hermes` directory to prevent log exposure/tampering in a shared temp path.</violation>
</file>
<file name="config/noctalia/ac-idle-inhibit.sh">
<violation number="1" location="config/noctalia/ac-idle-inhibit.sh:6">
P2: Resolve the AC status path dynamically instead of hard-coding `ACAD`; otherwise this service silently stops inhibiting idle on machines whose adapter is named differently.</violation>
</file>
<file name="Dockerfile">
<violation number="1" location="Dockerfile:54">
P2: Move `install.sh` below the warmup RUN so installer edits don't invalidate the nix-cache-warmup layer.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
|
|
||
| if [ -n "$NIX_GITHUB_TOKEN" ]; then | ||
| if [ -n "${NIX_CONFIG:-}" ]; then | ||
| NIX_CONFIG="${NIX_CONFIG} access-tokens = github.com=$NIX_GITHUB_TOKEN" |
Contributor
There was a problem hiding this comment.
P1: Preserve the newline separator here; a space can make the existing NIX_CONFIG entry and access-tokens collapse into one invalid line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install.sh, line 50:
<comment>Preserve the newline separator here; a space can make the existing NIX_CONFIG entry and `access-tokens` collapse into one invalid line.</comment>
<file context>
@@ -47,8 +47,7 @@ fi
if [ -n "${NIX_CONFIG:-}" ]; then
- NIX_CONFIG="${NIX_CONFIG}
-access-tokens = github.com=$NIX_GITHUB_TOKEN"
+ NIX_CONFIG="${NIX_CONFIG} access-tokens = github.com=$NIX_GITHUB_TOKEN"
else
NIX_CONFIG="access-tokens = github.com=$NIX_GITHUB_TOKEN"
</file context>
Suggested change
| NIX_CONFIG="${NIX_CONFIG} access-tokens = github.com=$NIX_GITHUB_TOKEN" | |
| NIX_CONFIG="${NIX_CONFIG} | |
| access-tokens = github.com=$NIX_GITHUB_TOKEN" |
Tip: Review your code locally with the cubic CLI to iterate faster.
| personalities: {} | ||
| security: | ||
| allow_private_urls: false | ||
| redact_secrets: false |
Contributor
There was a problem hiding this comment.
P1: Secret redaction is turned off by default, which can expose credentials in logs and tool output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/hermes/config.tpl.yaml, line 265:
<comment>Secret redaction is turned off by default, which can expose credentials in logs and tool output.</comment>
<file context>
@@ -0,0 +1,305 @@
+personalities: {}
+security:
+ allow_private_urls: false
+ redact_secrets: false
+ tirith_enabled: true
+ tirith_path: tirith
</file context>
|
|
||
| .PHONY: update | ||
| update: nix-update neovim-update gitalias-update llm-update overlays-update ## Update Nix flake, overlays, Neovim plugins, LLM configs, gitalias, and bun deps | ||
| update: neovim-update gitalias-update llm-update overlays-update ## Update Nix flake, overlays, Neovim plugins, LLM configs, gitalias, and bun deps |
Contributor
There was a problem hiding this comment.
P2: make update no longer runs nix-update, so it no longer updates Nix despite the target description saying it does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 302:
<comment>`make update` no longer runs `nix-update`, so it no longer updates Nix despite the target description saying it does.</comment>
<file context>
@@ -299,7 +299,7 @@ rtk-rewrite-sync: ## Sync rtk-rewrite.sh from upstream rtk repo.
.PHONY: update
-update: nix-update neovim-update gitalias-update llm-update overlays-update ## Update Nix flake, overlays, Neovim plugins, LLM configs, gitalias, and bun deps
+update: neovim-update gitalias-update llm-update overlays-update ## Update Nix flake, overlays, Neovim plugins, LLM configs, gitalias, and bun deps
.PHONY: update-lock
</file context>
Suggested change
| update: neovim-update gitalias-update llm-update overlays-update ## Update Nix flake, overlays, Neovim plugins, LLM configs, gitalias, and bun deps | |
| update: nix-update neovim-update gitalias-update llm-update overlays-update ## Update Nix flake, overlays, Neovim plugins, LLM configs, gitalias, and bun deps |
|
|
||
| # Hydrate config.yaml | ||
| @sed@ \ | ||
| -e "s|__CLIPROXY_API_KEY__|${CLIPROXY_API_KEY}|g" \ |
Contributor
There was a problem hiding this comment.
P2: Escape secret values before using them in sed replacements; unescaped &, |, or \ can break hydration and generate invalid config.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/hermes/hydrate.sh, line 73:
<comment>Escape secret values before using them in `sed` replacements; unescaped `&`, `|`, or `\` can break hydration and generate invalid config.</comment>
<file context>
@@ -0,0 +1,86 @@
+
+# Hydrate config.yaml
+@sed@ \
+ -e "s|__CLIPROXY_API_KEY__|${CLIPROXY_API_KEY}|g" \
+ "$CONFIG_TEMPLATE" >"${STATE_DIR}/config.yaml"
+chmod 600 "${STATE_DIR}/config.yaml"
</file context>
| "PATH=${homeDir}/.local/bin:${homeDir}/.nix-profile/bin:/usr/local/bin:/usr/bin:/bin" | ||
| ]; | ||
| WorkingDirectory = "${homeDir}/.hermes"; | ||
| StandardOutput = "append:/tmp/hermes/hermes-gateway.log"; |
Contributor
There was a problem hiding this comment.
P2: Avoid writing Hermes gateway logs to /tmp; use the private ${homeDir}/.hermes directory to prevent log exposure/tampering in a shared temp path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/hermes/default.nix, line 37:
<comment>Avoid writing Hermes gateway logs to `/tmp`; use the private `${homeDir}/.hermes` directory to prevent log exposure/tampering in a shared temp path.</comment>
<file context>
@@ -0,0 +1,44 @@
+ "PATH=${homeDir}/.local/bin:${homeDir}/.nix-profile/bin:/usr/local/bin:/usr/bin:/bin"
+ ];
+ WorkingDirectory = "${homeDir}/.hermes";
+ StandardOutput = "append:/tmp/hermes/hermes-gateway.log";
+ StandardError = "append:/tmp/hermes/hermes-gateway.log";
+ };
</file context>
| # Polls every 2s so AC state changes take effect well within the 5-min idle window. | ||
| set -euo pipefail | ||
|
|
||
| AC=/sys/class/power_supply/ACAD/online |
Contributor
There was a problem hiding this comment.
P2: Resolve the AC status path dynamically instead of hard-coding ACAD; otherwise this service silently stops inhibiting idle on machines whose adapter is named differently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/noctalia/ac-idle-inhibit.sh, line 6:
<comment>Resolve the AC status path dynamically instead of hard-coding `ACAD`; otherwise this service silently stops inhibiting idle on machines whose adapter is named differently.</comment>
<file context>
@@ -0,0 +1,14 @@
+# Polls every 2s so AC state changes take effect well within the 5-min idle window.
+set -euo pipefail
+
+AC=/sys/class/power_supply/ACAD/online
+
+while true; do
</file context>
- use local install.sh in Docker build and keep token handling stable - enforce cache warmup ordering in Makefile - add nix cache warmup script plus execution-style ShellSpec coverage Co-authored-by: Codex <noreply@openai.com>
shunkakinoki
force-pushed
the
fix/docker-cache-reuse
branch
from
May 5, 2026 02:35
9e1bbfa to
4dbfd64
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Makefile (1)
154-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce deterministic
installordering under parallel make.Line 154 defines
nix-build,nix-switch, andshell-installas sibling prerequisites. Undermake -j, they run concurrently;shell-installcan execute beforenix-switch, and sinceshell-installchecksif command -v fishwithout depending onnix-switchcompleting, fish setup is silently skipped if fish hasn't been installed yet.💡 Suggested fix
-.PHONY: install -install: setup git-submodule-sync nix-build nix-switch shell-install ## Set up full environment (setup, cache warmup, build, switch, shell-install). +.PHONY: install +install: setup git-submodule-sync ## Set up full environment (setup, cache warmup, build, switch, shell-install). + @$(MAKE) nix-build + @$(MAKE) nix-switch + @$(MAKE) shell-installThis pattern is already established in the
checktarget (lines 162–165).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 154, The install target allows nix-build, nix-switch, and shell-install to run in parallel under make -j, causing shell-install to run before nix-switch completes; make shell-install explicitly depend on nix-switch (i.e., add a rule such that shell-install has nix-switch as a prerequisite) so that nix-switch finishes before shell-install runs, mirroring the serialized ordering approach used for the check target and preventing fish/setup steps from being skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Makefile`:
- Line 154: The install target allows nix-build, nix-switch, and shell-install
to run in parallel under make -j, causing shell-install to run before nix-switch
completes; make shell-install explicitly depend on nix-switch (i.e., add a rule
such that shell-install has nix-switch as a prerequisite) so that nix-switch
finishes before shell-install runs, mirroring the serialized ordering approach
used for the check target and preventing fish/setup steps from being skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f75ba334-79b0-49c8-ab5b-2aa5c80237cc
📒 Files selected for processing (5)
DockerfileMakefileinstall.shscripts/nix-cache-warmup.shspec/nix_cache_warmup_spec.sh
✅ Files skipped from review due to trivial changes (1)
- scripts/nix-cache-warmup.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/nix_cache_warmup_spec.sh
- install.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation