Conversation
The launchd plist hard-coded PATH to /usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin and the systemd unit set none, so language servers installed elsewhere (for example /usr/local/share/dotnet or ~/go/bin) were not found by the supervised daemon, and a hand-edited PATH was lost on the next install-service. install-service now captures the absolute entries of the installing shell's PATH, without duplicates and in the shell's order, like xdgServiceEnv does for XDG_*. launchd appends any missing old defaults, so an empty PATH renders the old value. systemd gets Environment=PATH only when something was captured. No other variables are captured.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. Your free on-demand review promotion remains active until October 9, 2026 at 6:00 PM UTC. 📜 Recent review details⏰ Context from checks skipped due to timeout. (9)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (2)
📝 SummarySummary by CodeRabbit
WalkthroughService-unit generation captures absolute shell ChangesService PATH capture
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The service captures PATH as intended, with launchd defaults and escaped systemd output. No concrete issue remains that should block merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Essentials
Run ID: ae03604f-b8d6-4705-8dd2-243d19dba426
📒 Files selected for processing (2)
cmd/gortex/daemon_service.gocmd/gortex/daemon_service_test.go
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. Your free on-demand review promotion remains active until October 9, 2026 at 6:00 PM UTC.
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: test (ubuntu-latest, 1.27)
- GitHub Check: test (macos-latest, 1.27)
- GitHub Check: lint
- GitHub Check: build-linux-static
- GitHub Check: trivy-fs
- GitHub Check: benchmark
- GitHub Check: test (windows-latest, 1.27)
- GitHub Check: build-onnx
- GitHub Check: govulncheck
- GitHub Check: skill-drift
A captured PATH entry with a line break would split the systemd unit's Environment= line into a separate directive. Such an entry cannot name a usable directory, so servicePath now skips it like a relative entry.
systemd.syntax(7) recognizes a quote only at the start of an item, so Environment=KEY="a b" does not unquote the value. When a captured PATH or XDG_* value contains whitespace, quote the complete KEY=value assignment instead, as systemd.exec(5) shows: Environment="KEY=a b".
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Essentials
- Run ID:
bf2162ee-7ca0-4e42-bfc3-4c9ca37d16aa
📒 Files selected for processing (2)
cmd/gortex/daemon_service.gocmd/gortex/daemon_service_test.go
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. Your free on-demand review promotion remains active until October 9, 2026 at 6:00 PM UTC.
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: benchmark
- GitHub Check: govulncheck
- GitHub Check: test (windows-latest, 1.27)
- GitHub Check: test (ubuntu-latest, 1.27)
- GitHub Check: build-linux-static
- GitHub Check: test (macos-latest, 1.27)
- GitHub Check: lint
- GitHub Check: skill-drift
- GitHub Check: build-onnx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: peterkc
Repo: peterkc/gortex
Timestamp: 2026-10-02T22:30:45.711Z
Learning: In cmd/gortex/daemon_service.go, servicePath deliberately drops PATH entries containing CR or LF rather than failing install-service. This capture policy excludes those entries from both systemd units and launchd plists. Do not justify this policy by claiming that Unix directory names cannot contain CR or LF.
systemd applies C-style escapes to unquoted Environment= text, so a PATH entry such as /opt/a\tools would reach the daemon with a tab in it. An unquoted quote character would start a quoted section. systemdEnvValue now also quotes the whole assignment when it contains a backslash or a quote, and escapes backslashes and double quotes inside the quotes.
Owner
Author
|
Continued upstream as zzet#864. |
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
gortex daemon install-servicewrites the launchd plist with a fixedPATH(/usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin) and the systemd unit with noPATH. The supervised daemon then cannot find language servers installed elsewhere, such asdotnetin/usr/local/share/dotnetor tools in~/go/bin. A user who adds them to the plist by hand loses them on the nextinstall-service.This change captures the installing shell's
PATHinto the unit, the same wayxdgServiceEnvalready capturesXDG_*.Changes
servicePathhelper indaemon_service.go. It keeps only absolute entries, because an empty or relative entry such as.would resolve against the service's working directory. It drops duplicates and keeps the shell's order. Entries that do not exist yet are kept, so a tool installed later is still found.PATHrenders the old value exactly.PATHassignment goes throughsystemdEnvValueand is written only when something was captured. Otherwise systemd's default applies, as before. The macOS defaults are not added on Linux.GORTEX_DAEMON_HTTP_TOKENstays out of the plist, which is written with mode0644. A test checks this.XDG_*, re-runinstall-serviceto pick up a changedPATH.servicePathdrops an entry that contains CR or LF, because it would split the systemdEnvironment=line.KEY=valueassignment (Environment="PATH=/a b"), because systemd recognizes a quote only at the start of an item. This also applies to the existingXDG_*lines, and theTestRenderSystemdUnit_PropagatesXDGassertion changes to that form.\tto unquoted text and treats an unquoted quote as the start of a quoted section.TestRenderSystemdUnit_NoXDGnow sets an emptyPATH, so it still tests the "nothing captured" case instead of inheriting the test runner'sPATH. Its assertions are unchanged.Testing
go test -race ./...)go test -race -run 'TestRenderLaunchdPlist|TestRenderSystemdUnit|TestXDGServiceEnv|TestServicePath|TestSystemdEnvValue' ./cmd/gortex/,go vetandgolangci-lint run ./cmd/gortex/pass. When the defaults are put before the shell's entries, 2 of the new tests fail. Not tested: a live service install on macOS or Linux.Checklist
Meta["methods"]for interfaces (if applicable)EdgeMemberOfedges to their containing type (if applicable)