Dev#35
Merged
Merged
Conversation
📝 WalkthroughWalkthroughA new Linux-only Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "CLI Parser"
participant Validator as "Arg Validator"
participant Generator as "Bash Command Generator"
participant Executor as "Process Executor"
participant Systemd as "systemd"
participant Nginx as "nginx"
User->>CLI: shiphook cleanup --domain example.com
CLI->>Validator: parseCleanupTarget(argv)
Validator-->>CLI: { mode: "domain", domain }
CLI->>Generator: buildCleanupBashCommand(target)
Generator->>Generator: create timestamped backup dir
Generator-->>CLI: bash command string
CLI->>Executor: spawnSync("sudo bash -lc <cmd>")
Executor->>Systemd: inspect/disable/remove matching units
Executor->>Nginx: move matching configs to backup
Executor->>Nginx: nginx -t and reload
Executor-->>CLI: exit code 0/1
CLI-->>User: success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/cleanup.ts`:
- Around line 52-59: The systemdCleanup array currently removes all
shiphook*.service units regardless of --domain mode, which is inconsistent with
domain-scoped nginx cleanup; update the cleanup to be domain-aware by changing
the systemd command in systemdCleanup (or the function that uses it) to filter
unit files for the target domain (e.g., inspect unit contents or names for the
domain string and only disable/remove matching units) instead of unconditionally
disabling/removing all shiphook*.service units; update references to systemctl
disable/--now, rm -f /etc/systemd/system/shiphook*.service, and any
daemon-reload/reset-failed logic to operate only on the filtered set (or
alternately add explicit comments/docs if you decide to keep global removal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8299bb50-2a6e-4551-9bf5-d3e30cb6d874
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddocs/deployment-recipes.mddocs/self-hosted-https.mddocs/systemd.mdpackage.jsonsrc/cleanup.test.tssrc/cleanup.tssrc/cli.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cleanup.ts (1)
114-118: Consider whether login shell (-l) is necessary.
bash -lcruns a login shell, which sources profile scripts like/etc/profileand~/.bash_profile. This could introduce unexpected behavior depending on root's shell configuration. If the intent is just PATH setup,bash -cwith explicit PATH may be more predictable—though in practice,sudotypically preserves a safe PATH.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cleanup.ts` around lines 114 - 118, The current runCleanup function invokes spawnSync with "bash -lc", which starts a login shell and can source profile scripts unexpectedly; change the invocation to use "bash -c" instead (i.e., replace the "-lc" flag with "-c") so the command runs without forcing a login shell, and if you need specific PATH or environment for the cleanup commands, explicitly set it via the spawn options (env) or prepend PATH in the command built by buildCleanupBashCommand; update references in runCleanup and ensure buildCleanupBashCommand still produces a safe, self-contained command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cleanup.ts`:
- Around line 43-52: The current unitMatchesDomain uses simple substring checks
which cause false positives (e.g., "a.com" matching "data.com"); update
unitMatchesDomain to perform stricter matching — either use a
word-boundary/anchored regex (e.g., /\bDOMAIN\b/ or match lines like
/^server_name\s+.*\bDOMAIN\b/i against unitContent) and enforce a clear unit
name pattern (e.g., exact name, prefix/suffix, or a regex) instead of includes;
also update the corresponding shell grep -Fq usage to the same stricter check so
both unitMatchesDomain and the bash check use the same exact-match semantics
(reference functions: unitMatchesDomain and selectDomainUnitsForCleanup).
---
Nitpick comments:
In `@src/cleanup.ts`:
- Around line 114-118: The current runCleanup function invokes spawnSync with
"bash -lc", which starts a login shell and can source profile scripts
unexpectedly; change the invocation to use "bash -c" instead (i.e., replace the
"-lc" flag with "-c") so the command runs without forcing a login shell, and if
you need specific PATH or environment for the cleanup commands, explicitly set
it via the spawn options (env) or prepend PATH in the command built by
buildCleanupBashCommand; update references in runCleanup and ensure
buildCleanupBashCommand still produces a safe, self-contained command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2ee34d1-3bed-4c1c-9508-dbe9e5d86659
📒 Files selected for processing (4)
README.mddocs/systemd.mdsrc/cleanup.test.tssrc/cleanup.ts
✅ Files skipped from review due to trivial changes (2)
- README.md
- src/cleanup.test.ts
Comment on lines
+43
to
+52
| export function unitMatchesDomain(unitName: string, unitContent: string, domain: string): boolean { | ||
| return unitName.includes(domain) || unitContent.includes(domain); | ||
| } | ||
|
|
||
| export function selectDomainUnitsForCleanup( | ||
| units: Array<{ name: string; content: string }>, | ||
| domain: string | ||
| ): string[] { | ||
| return units.filter((u) => unitMatchesDomain(u.name, u.content, domain)).map((u) => u.name); | ||
| } |
There was a problem hiding this comment.
Substring matching may cause unintended matches.
The includes() check will match a domain like "a.com" against unit names or content containing "data.com". While the backup mechanism provides a safety net, consider using word-boundary matching or requiring the domain to appear as a distinct server_name or in the unit name pattern.
This mirrors the grep -Fq behavior in the bash command (line 93), so both would need updating if addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cleanup.ts` around lines 43 - 52, The current unitMatchesDomain uses
simple substring checks which cause false positives (e.g., "a.com" matching
"data.com"); update unitMatchesDomain to perform stricter matching — either use
a word-boundary/anchored regex (e.g., /\bDOMAIN\b/ or match lines like
/^server_name\s+.*\bDOMAIN\b/i against unitContent) and enforce a clear unit
name pattern (e.g., exact name, prefix/suffix, or a regex) instead of includes;
also update the corresponding shell grep -Fq usage to the same stricter check so
both unitMatchesDomain and the bash check use the same exact-match semantics
(reference functions: unitMatchesDomain and selectDomainUnitsForCleanup).
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 by CodeRabbit
New Features
shiphook cleanup --domain <host>andshiphook cleanup --all.Documentation
Tests
Chores