fix(ci): repair Kyber activation and main checks - #2274
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates CI-specific dotagents synchronization, adds ripgrep to the development shell, adjusts CAAM shell conditions, substitutes the firewall’s Awk path through Nix, and updates ShellSpec lint directives and assertions. ChangesTooling and shell services
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| .PHONY: dotagents-sync | ||
| dotagents-sync: ## Sync dotagents (commands, skills, MCP configuration). | ||
| @$(MAKE) -C dotagents sync | ||
| @if [ "$$CI" = "true" ]; then \ |
There was a problem hiding this comment.
Latent — CI detection is too strict: GitHub Actions sets CI=true so this works today, but other CI providers set CI=1 (CircleCI) or other truthy values. On those, the guard falls through to $(MAKE) -C dotagents sync, which is exactly what was failing before this PR. Safer to check [ -n "$$CI" ] (or a case for known truthy values) so the skip triggers on any CI runner.
| @@ -1,4 +1,5 @@ | |||
| #!/usr/bin/env bash | |||
| # shellcheck disable=SC2016 | |||
There was a problem hiding this comment.
Nit: This disable is file-wide, so any future single-quoted $VAR typo elsewhere in the script won't be flagged. The only lines that legitimately need SC2016 suppressed are the two @awk@ '...' blocks (lines 46 and 77). Scoping the disable to those specific lines keeps the rest of the file lint-covered.
| End | ||
|
|
||
| It 'uses nix-substituted awk' | ||
| When run grep '@awk@' "$SCRIPT" |
There was a problem hiding this comment.
Nit: Every other assertion in this Describe wraps its command in bash -c "..." (see lines 52-77). Using bash -c "grep '@awk@' '$SCRIPT'" here keeps the file uniform — both forms work in ShellSpec, but consistency avoids future confusion.
| When run grep '@awk@' "$SCRIPT" | |
| When run bash -c "grep '@awk@' '$SCRIPT'" |
![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 6 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="home-manager/services/firewall/activate.sh">
<violation number="1" location="home-manager/services/firewall/activate.sh:2">
P3: This `shellcheck disable=SC2016` is applied file-wide, which disables the single-quoted variable check for the entire script rather than just the two `@awk@ '...'` blocks that legitimately need it (around lines 46 and 77). This means a genuine typo elsewhere in the file (e.g. an unintended literal `$VAR` inside single quotes) would no longer be flagged. Scope the disable directive to just the two awk lines instead.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -1,4 +1,5 @@ | |||
| #!/usr/bin/env bash | |||
| # shellcheck disable=SC2016 | |||
There was a problem hiding this comment.
P3: This shellcheck disable=SC2016 is applied file-wide, which disables the single-quoted variable check for the entire script rather than just the two @awk@ '...' blocks that legitimately need it (around lines 46 and 77). This means a genuine typo elsewhere in the file (e.g. an unintended literal $VAR inside single quotes) would no longer be flagged. Scope the disable directive to just the two awk lines instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/firewall/activate.sh, line 2:
<comment>This `shellcheck disable=SC2016` is applied file-wide, which disables the single-quoted variable check for the entire script rather than just the two `@awk@ '...'` blocks that legitimately need it (around lines 46 and 77). This means a genuine typo elsewhere in the file (e.g. an unintended literal `$VAR` inside single quotes) would no longer be flagged. Scope the disable directive to just the two awk lines instead.</comment>
<file context>
@@ -1,4 +1,5 @@
#!/usr/bin/env bash
+# shellcheck disable=SC2016
# Converge Kyber WAN firewall on every activation (IPv4 + IPv6).
# Public SSH is denied on the WAN NIC; access is via Tailscale (and Latitude SG).
</file context>
![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
🤖 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.
Inline comments:
In `@spec/activate_firewall_spec.sh`:
- Around line 79-82: Strengthen the “uses nix-substituted awk” test by verifying
replaceVars wiring rather than only checking the `@awk`@ token in the source
template. Add an assertion covering the awk configuration in the firewall
service definition, or instead run the generated activation script and verify
its substituted awk path; keep the existing template-token check only as
supplemental coverage.
🪄 Autofix
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 Plus
Run ID: e4cc0fd5-9dc5-49ac-a99b-05425d166e1e
📒 Files selected for processing (10)
Makefiledevenv.nixhome-manager/services/caam/setup.shhome-manager/services/caam/sync.shhome-manager/services/firewall/activate.shhome-manager/services/firewall/default.nixspec/activate_firewall_spec.shspec/activate_roborev_spec.shspec/caam_sync_service_spec.shspec/dotfiles_updater_spec.sh
| It 'uses nix-substituted awk' | ||
| When run grep '@awk@' "$SCRIPT" | ||
| The output should include '@awk@' | ||
| End |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Test the replaceVars wiring, not only the template token.
SCRIPT points to the source template, so this assertion passes whenever @awk@ remains in activate.sh. It does not detect removal or misconfiguration of awk = "${pkgs.gawk}/bin/awk" in home-manager/services/firewall/default.nix. Add an assertion for that wiring or test the generated activation script.
🤖 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 `@spec/activate_firewall_spec.sh` around lines 79 - 82, Strengthen the “uses
nix-substituted awk” test by verifying replaceVars wiring rather than only
checking the `@awk`@ token in the source template. Add an assertion covering the
awk configuration in the firewall service definition, or instead run the
generated activation script and verify its substituted awk path; keep the
existing template-token check only as supplemental coverage.
1 participant
Summary
gawkpath into the Kyber firewall activation scriptripgrepand isolate FishTape from host CAAM rotation stateValidation
make shell-checkmake shell-test-dev(1,931 ShellSpec examples; 447 FishTape tests)nixfmt --check home-manager/services/firewall/default.nixaarch64-darwinand Kyber isx86_64-linuxSummary by cubic
Repairs Kyber firewall activation by injecting Nix
gawk, stabilizes main checks by making dotagents sync deterministic in CI, and applies repository formatting to scripts and specs.Bug Fixes
gawkinto the firewall activation script; add substitution coverage in tests.Dependencies
ripgrepto the dev shell.Written for commit 56e4ce4. Summary will update on new commits.