Add nectar login, fleet login deferral, and lifecycle parity (PRD-003a/003b)#25
Merged
Merged
Conversation
…a/003b) nectar login: an in-process, zero-dependency Deeplake device-flow client (src/auth/device-flow.ts) that prints the verification URL and user code before any browser-open attempt, validates https before opening, honors poll floors and slow_down with a bounded cap, selects tenancy explicitly (prompt or --org/--workspace, never a silent orgs[0]), and writes ~/.deeplake/credentials.json byte-compatible with honeycomb's DiskCredentials via an atomic 0600 temp+rename. Solo-vs-fleet detection mirrors honeycomb's three-signal contract (registry entry, 3853 probe, npm global tree; any signal defers login to hive and the fired signals are logged). A daemon without credentials now serves 503 degraded with a machine-readable reason and flips healthy without restart when credentials appear (new credentials watch); nectar install auto-opens the popup only when solo with no credentials. Lifecycle: bare start/stop verbs front the OS service managers or the PID/lock files, and uninstall runs the three-part contract (unit current + legacy, registry entry, state dir) with honest exit codes: already-absent units are friendly no-ops, genuine removal failures exit nonzero naming the error. Security and QA audited (records under library/qa/); 828 tests green.
📝 WalkthroughWalkthroughThis PR implements PRD-003 fleet lifecycle features: device-flow OAuth login, solo/fleet detection heuristics, atomic Deep Lake credentials persistence with a polling CredentialsWatch, health storage-reachability tracking, service start/stop/uninstall lifecycle orchestration, Doctor registry deregistration, expanded CLI verbs, and QA/security audit documentation. ChangesFleet Lifecycle Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant classifyFleet
participant runDeviceFlowLogin
participant AuthAPI
participant Credentials
participant Daemon
participant HealthState
CLI->>classifyFleet: detect solo/fleet
classifyFleet-->>CLI: mode
CLI->>runDeviceFlowLogin: login (if solo, no creds)
runDeviceFlowLogin->>AuthAPI: device code + poll token
AuthAPI-->>runDeviceFlowLogin: long-lived token
runDeviceFlowLogin->>Credentials: saveDeepLakeCredentials
CLI->>Daemon: boot with storageCredentialsPresent + credentialsWatch
Daemon->>HealthState: setStorageState (initial)
loop credentials watch
Daemon->>Credentials: probe()
Daemon->>HealthState: setStorageState (on change)
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon.ts (1)
1074-1088: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop
credentialsWatchbeforeactiveProjectsController.credentialsWatch.onChange()can still queueactiveProjectsController.reconcileNow()whileactiveProjectsController.stop()is tearing the supervisor down, andreconcile()remains live afterstopAll(). Swap the order so shutdown cannot restart contexts mid-teardown.🤖 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 `@src/daemon.ts` around lines 1074 - 1088, The shutdown order in the stop logic is reversed: credentialsWatch can still enqueue activeProjectsController.reconcileNow() while activeProjectsController.stop() is tearing down. In the stopAll/daemon shutdown flow, move the credentialsWatch.stop() block before activeProjectsController.stop(), using the existing credentialsWatch and activeProjectsController references, so no reconcile can restart contexts mid-teardown.
🧹 Nitpick comments (4)
src/cli.ts (1)
161-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing
lifecycleServiceModule()inrunInstall().
runInstall()(unchanged lines 330-333) builds an identicalServiceModuleviacreateServiceModule({ execPath: resolveServiceExecPath(), preferSystemScope: preferSystemScope() }). Now that this PR introduceslifecycleServiceModule()for the same purpose, reusing it inrunInstall()would remove the duplication.🤖 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 `@src/cli.ts` around lines 161 - 164, runInstall() still constructs the same ServiceModule inline that lifecycleServiceModule() already builds, so remove the duplication by reusing lifecycleServiceModule() there. Update the runInstall flow to call lifecycleServiceModule() instead of repeating createServiceModule({ execPath: resolveServiceExecPath(), preferSystemScope: preferSystemScope() }), keeping the shared module creation logic in one place.test/cli-verbs.test.ts (1)
15-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider behavior-based assertions over source-text regex matching.
Lines 15-30 assert against
dist/cli.js's raw source text rather than actually invokingmain()with each verb (as Line 32-50 already does for--help). This only proves the string exists in the dispatch table, not that the verb dispatches/executes correctly. Given this mirrors an existing pattern elsewhere in the codebase, this is optional.🤖 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 `@test/cli-verbs.test.ts` around lines 15 - 30, The tests in cli-verbs.test.ts are matching raw source text in dist/cli.js instead of verifying behavior, so update the assertions to exercise main() with each verb and check the resulting dispatch/handler outcome. Use the existing main() help-invocation pattern in this spec as the model, and cover status/service-status plus login/start/stop/uninstall/daemon/install by invoking the CLI entrypoint directly rather than asserting regexes on the bundled source.test/install-login-decision.test.ts (1)
23-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the empty-value
--org=/--workspace=case.Once the empty-value validation is added in
parseLoginFlags(see src/cli.ts review), a test likeparseLoginFlags(["--org="])asserting an error would lock in that behavior.🤖 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 `@test/install-login-decision.test.ts` around lines 23 - 36, Add a test in install-login-decision.test.ts for the empty-value flag form handled by parseLoginFlags. Extend the existing parseLoginFlags cases to cover inputs like "--org=" and "--workspace=" and assert they produce an error instead of a flag value, alongside the existing missing-value and unknown-flag checks. Use the parseLoginFlags test block so the new behavior is locked in when the src/cli.ts validation is implemented.test/lifecycle.test.ts (1)
159-292: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for a failed daemon stop during uninstall.
No test exercises
service.stop()failing (orsendSignalreturning false) whileisDaemonRunning()is true duringrunUninstallLifecycleorrunStopLifecycle. See the corresponding findings insrc/lifecycle.ts(step 1 ofrunUninstallLifecycle, and the ambiguous-message branch ofrunStopLifecycle) — a test here would have caught both.🤖 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 `@test/lifecycle.test.ts` around lines 159 - 292, Add a test that covers daemon stop failure during uninstall by running runUninstallLifecycle with isDaemonRunning() true and either service.stop() failing or sendSignal() returning false, then assert the lifecycle exits nonzero and emits the expected stop-failure message. Use the existing runUninstallLifecycle, runStopLifecycle, fakeService, and io helpers so the new coverage also protects the ambiguous-message branch in src/lifecycle.ts.
🤖 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 `@library/qa/security/2026-07-04-security-audit-prd-003-fleet-lifecycle.md`:
- Line 23: The Markdown table row is broken by unescaped regex alternation pipes
inside the `isAlreadyAbsentFailure` description, causing the column count to
render incorrectly. Update the security audit entry so the regex text no longer
uses raw `|` characters in the table cell, either by escaping them or moving the
pattern out of the table. Keep the wording tied to `isAlreadyAbsentFailure` and
the locale-independent checks, but ensure the row renders as a valid single cell
per column.
In `@src/auth/device-flow.ts`:
- Around line 258-272: The reMint() flow currently uses request() for POST
/users/me/tokens, which can retry 5xx responses and accidentally mint duplicate
org tokens if the first response was lost. Update reMint() in the device-flow
token minting path to use non-retrying behavior, or restrict retries to
429-only, so the token creation request is not replayed on server errors. Keep
the change localized to the reMint() call site and preserve the existing
authHeaders, MINT_DURATION_SECONDS, and AuthHttpError handling.
In `@src/lifecycle.ts`:
- Around line 101-117: The fallback message in runStopLifecycle is misleading
when isDaemonRunning() confirms the daemon is alive but neither service.stop()
nor sendSignal() succeeds. Update the logic in runStopLifecycle to distinguish
the “known running but could not stop” case from the “nothing was running” case,
and emit a message that reflects the confirmed running state when running is
true and pid is null. Keep the 0 exit code behavior unchanged, but adjust the
conditional around viaService, pid, and signalled so the output matches the
actual daemon state.
- Around line 179-188: `runUninstallLifecycle()` currently reports the daemon as
stopped based only on `isDaemonRunning()`, even if both `service.stop()` and
`sendSignal()` fail. Update the uninstall flow to check the outcome of those
stop attempts in `runUninstallLifecycle` and only print the success message when
at least one stop action succeeds; if both fail, set `hardFailure` or otherwise
surface the failure and avoid claiming the daemon stopped while proceeding
through uninstall.
---
Outside diff comments:
In `@src/daemon.ts`:
- Around line 1074-1088: The shutdown order in the stop logic is reversed:
credentialsWatch can still enqueue activeProjectsController.reconcileNow() while
activeProjectsController.stop() is tearing down. In the stopAll/daemon shutdown
flow, move the credentialsWatch.stop() block before
activeProjectsController.stop(), using the existing credentialsWatch and
activeProjectsController references, so no reconcile can restart contexts
mid-teardown.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 161-164: runInstall() still constructs the same ServiceModule
inline that lifecycleServiceModule() already builds, so remove the duplication
by reusing lifecycleServiceModule() there. Update the runInstall flow to call
lifecycleServiceModule() instead of repeating createServiceModule({ execPath:
resolveServiceExecPath(), preferSystemScope: preferSystemScope() }), keeping the
shared module creation logic in one place.
In `@test/cli-verbs.test.ts`:
- Around line 15-30: The tests in cli-verbs.test.ts are matching raw source text
in dist/cli.js instead of verifying behavior, so update the assertions to
exercise main() with each verb and check the resulting dispatch/handler outcome.
Use the existing main() help-invocation pattern in this spec as the model, and
cover status/service-status plus login/start/stop/uninstall/daemon/install by
invoking the CLI entrypoint directly rather than asserting regexes on the
bundled source.
In `@test/install-login-decision.test.ts`:
- Around line 23-36: Add a test in install-login-decision.test.ts for the
empty-value flag form handled by parseLoginFlags. Extend the existing
parseLoginFlags cases to cover inputs like "--org=" and "--workspace=" and
assert they produce an error instead of a flag value, alongside the existing
missing-value and unknown-flag checks. Use the parseLoginFlags test block so the
new behavior is locked in when the src/cli.ts validation is implemented.
In `@test/lifecycle.test.ts`:
- Around line 159-292: Add a test that covers daemon stop failure during
uninstall by running runUninstallLifecycle with isDaemonRunning() true and
either service.stop() failing or sendSignal() returning false, then assert the
lifecycle exits nonzero and emits the expected stop-failure message. Use the
existing runUninstallLifecycle, runStopLifecycle, fakeService, and io helpers so
the new coverage also protects the ambiguous-message branch in src/lifecycle.ts.
🪄 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 Plus
Run ID: b2c13d92-8eb3-49d3-9c7f-2d7139c750be
📒 Files selected for processing (23)
library/qa/quality/2026-07-05-qa-report-prd-003-fleet-lifecycle.mdlibrary/qa/security/2026-07-04-security-audit-prd-003-fleet-lifecycle.mdsrc/auth/device-flow.tssrc/cli.tssrc/credentials-watch.tssrc/daemon.tssrc/doctor-registry.tssrc/fleet-detection.tssrc/health.tssrc/hive-graph/deeplake-credentials.tssrc/lifecycle.tssrc/service/argv.tssrc/service/index.tstest/cli-verbs.test.tstest/credentials-writer.test.tstest/device-flow-login.test.tstest/doctor-registry-deregister.test.tstest/fleet-detection.test.tstest/fleet-lifecycle-daemon.test.tstest/fleet-lifecycle-health.test.tstest/install-login-decision.test.tstest/lifecycle.test.tstest/service-start-stop.test.ts
| | M1 | Medium | `src/fleet-detection.ts:154-175` (pre-fix) | The S3 npm probe ran `execFile("npm.cmd", ..., { shell: false })` on win32. Since Node's CVE-2024-27980 hardening (all Node >= 20.12, and this repo requires >= 22), spawning a `.cmd` with `shell: false` throws EINVAL, so the probe's catch resolved `false` on EVERY Windows machine - silently blinding one of the three signals that suppress the auto sign-in popup, weakening the fleet-detection control on the product's primary platform. | **Fixed**: `shell: true` on win32 only. Safe here because every argv element is a compile-time constant (`ls -g @legioncodeinc/hive --depth 0`); no user input reaches the command line. Non-Windows platforms keep `shell: false`. | | ||
| | M2 | Medium | `src/auth/device-flow.ts:481` and `:218` (pre-fix) | Server-derived strings reached the terminal unvalidated: the PRINTED `verification_uri` skipped the https-only check (only the browser-open path validated), and `verification_uri`, `user_code`, and truncated HTTP error bodies were echoed raw, allowing ANSI/control-character terminal escape injection from a compromised or spoofed auth response. Printing a non-https URL also instructs the user to open it manually, sidestepping the opener's https guard. | **Fixed**: the printed URL now goes through `validateVerificationUrl` and the flow refuses (clear `{ ok: false }` message, no hang) on a non-https `verification_uri`; a new `sanitizeForTerminal` strips C0/C1 control characters (including ESC) from the printed user code and from the truncated error bodies carried in `AuthHttpError`. | | ||
| | L1 | Low | `src/hive-graph/deeplake-credentials.ts`, `src/doctor-registry.ts:217` | Predictable temp-file names (`pid` + timestamp) for the atomic writes. | Documented, accepted: consistent with the prior audit's accepted posture (2026-07-04 L4); the credentials dir is owner-only (`0700`) and the registry dir lives under the user-owned fleet root; rename is atomic. | | ||
| | L2 | Low | `src/service/index.ts` `isAlreadyAbsentFailure` | The generic case-insensitive text fallback (`does not exist|cannot find|no such process|not[- ]loaded|could not find|not found`) could misclassify a GENUINE uninstall failure whose stderr coincidentally contains one of those phrases (for example a "config file not found" sub-error) as already-absent, masking a boot-resurrecting unit while `nectar uninstall` reports a friendly no-op. The locale-independent numeric checks (sc 1060, launchd exit 3) are correct. | Documented, accepted for now: the misclassification window is narrow, the conservative default (anything unmatched is a genuine failure) points the right way, and a per-manager pre-flight status query would be the robust follow-up. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the regex pipes in this table row.
The | alternation inside the cell breaks the Markdown table, so the row will render with the wrong column count. Move the regex text out of the table or replace the pipes with a safe separator.
🛠️ Suggested fix
-| L2 | Low | `src/service/index.ts` `isAlreadyAbsentFailure` | The generic case-insensitive text fallback (`does not exist|cannot find|no such process|not[- ]loaded|could not find|not found`) could misclassify a GENUINE uninstall failure whose stderr coincidentally contains one of those phrases (for example a "config file not found" sub-error) as already-absent, masking a boot-resurrecting unit while `nectar uninstall` reports a friendly no-op. The locale-independent numeric checks (sc 1060, launchd exit 3) are correct. | Documented, accepted for now: the misclassification window is narrow, the conservative default (anything unmatched is a genuine failure) points the right way, and a per-manager pre-flight status query would be the robust follow-up. |
+| L2 | Low | `src/service/index.ts` `isAlreadyAbsentFailure` | The generic case-insensitive text fallback (`does not exist / cannot find / no such process / not[- ]loaded / could not find / not found`) could misclassify a GENUINE uninstall failure whose stderr coincidentally contains one of those phrases (for example a "config file not found" sub-error) as already-absent, masking a boot-resurrecting unit while `nectar uninstall` reports a friendly no-op. The locale-independent numeric checks (sc 1060, launchd exit 3) are correct. | Documented, accepted for now: the misclassification window is narrow, the conservative default (anything unmatched is a genuine failure) points the right way, and a per-manager pre-flight status query would be the robust follow-up. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | L2 | Low | `src/service/index.ts` `isAlreadyAbsentFailure` | The generic case-insensitive text fallback (`does not exist|cannot find|no such process|not[- ]loaded|could not find|not found`) could misclassify a GENUINE uninstall failure whose stderr coincidentally contains one of those phrases (for example a "config file not found" sub-error) as already-absent, masking a boot-resurrecting unit while `nectar uninstall` reports a friendly no-op. The locale-independent numeric checks (sc 1060, launchd exit 3) are correct. | Documented, accepted for now: the misclassification window is narrow, the conservative default (anything unmatched is a genuine failure) points the right way, and a per-manager pre-flight status query would be the robust follow-up. | | |
| | L2 | Low | `src/service/index.ts` `isAlreadyAbsentFailure` | The generic case-insensitive text fallback (`does not exist / cannot find / no such process / not[- ]loaded / could not find / not found`) could misclassify a GENUINE uninstall failure whose stderr coincidentally contains one of those phrases (for example a "config file not found" sub-error) as already-absent, masking a boot-resurrecting unit while `nectar uninstall` reports a friendly no-op. The locale-independent numeric checks (sc 1060, launchd exit 3) are correct. | Documented, accepted for now: the misclassification window is narrow, the conservative default (anything unmatched is a genuine failure) points the right way, and a per-manager pre-flight status query would be the robust follow-up. | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 23-23: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 23-23: Table column count
Expected: 5; Actual: 10; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@library/qa/security/2026-07-04-security-audit-prd-003-fleet-lifecycle.md` at
line 23, The Markdown table row is broken by unescaped regex alternation pipes
inside the `isAlreadyAbsentFailure` description, causing the column count to
render incorrectly. Update the security audit entry so the regex text no longer
uses raw `|` characters in the table cell, either by escaping them or moving the
pattern out of the table. Keep the wording tied to `isAlreadyAbsentFailure` and
the locale-independent checks, but ensure the row renders as a valid single cell
per column.
Source: Linters/SAST tools
Comment on lines
+258
to
+272
| async reMint(token: string, orgId: string): Promise<string> { | ||
| const name = `nectar-cli-${Date.now()}`; | ||
| const body = await request( | ||
| "/users/me/tokens", | ||
| { | ||
| method: "POST", | ||
| headers: authHeaders(token), | ||
| body: JSON.stringify({ name, duration: MINT_DURATION_SECONDS, organization_id: orgId }), | ||
| }, | ||
| true, | ||
| ); | ||
| const minted = (body as { token?: { token?: unknown } } | null)?.token?.token; | ||
| if (typeof minted !== "string" || minted.length === 0) throw new AuthHttpError(0, "auth API minted no token"); | ||
| return minted; | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Deep Lake api.deeplake.ai POST /users/me/tokens idempotency device flow token mint
💡 Result:
Deep Lake authentication relies on API tokens (JWTs) generated through the Deep Lake App [1][2]. Users obtain these tokens by navigating to Settings > API Tokens within their workspace at deeplake.ai [1][2]. There is no public or documented "device flow" for minting tokens via the POST /users/me/tokens endpoint in the standard Deep Lake API reference [3][4]. The documented method for authentication requires users to manually create tokens via the web interface and provide them via the Authorization: Bearer header or by setting the DEEPLAKE_API_KEY environment variable for SDK and REST API operations [1][3][2]. The Deep Lake API does not specify idempotency requirements for token-related endpoints in its public documentation [3][4]. Standard API interactions, such as creating tables or running queries, utilize the token for authorization, and the SDK automatically handles extracting organization information from the token claims [1][2]. If an API token expires or is invalid, the API returns a 401 Unauthorized error, and the standard resolution is to generate a new token via the web dashboard [1][2].
Citations:
- 1: https://docs.deeplake.ai/4.6/guide/authentication/
- 2: https://docs.deeplake.ai/4.5/guide/authentication/
- 3: https://docs.deeplake.ai/4.5/reference/rest-api/
- 4: https://docs.deeplake.ai/4.5/reference/client/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and locate the relevant symbols.
ast-grep outline src/auth/device-flow.ts --view expanded || true
printf '\n--- request() / reMint() / pollDeviceToken() matches ---\n'
rg -n -C 4 'async function request|async reMint|reMint\(|pollDeviceToken|MINT_DURATION_SECONDS|AuthHttpError' src/auth/device-flow.ts
printf '\n--- relevant chunk around request() and reMint() ---\n'
sed -n '180,290p' src/auth/device-flow.ts
printf '\n--- call sites for reMint() ---\n'
rg -n -C 3 '\breMint\(' src/auth/device-flow.tsRepository: legioncodeinc/nectar
Length of output: 18737
Avoid retrying the token-mint POST reMint() goes through request(), which retries 5xx responses. If /users/me/tokens succeeds server-side but the response is lost, the retry can mint a second long-lived org token and leave the first one orphaned. Limit this call to non-retrying behavior or 429-only retries.
🤖 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 `@src/auth/device-flow.ts` around lines 258 - 272, The reMint() flow currently
uses request() for POST /users/me/tokens, which can retry 5xx responses and
accidentally mint duplicate org tokens if the first response was lost. Update
reMint() in the device-flow token minting path to use non-retrying behavior, or
restrict retries to 429-only, so the token creation request is not replayed on
server errors. Keep the change localized to the reMint() call site and preserve
the existing authHeaders, MINT_DURATION_SECONDS, and AuthHttpError handling.
Comment on lines
+101
to
+117
| export async function runStopLifecycle(deps: StopLifecycleDeps): Promise<number> { | ||
| const running = deps.isDaemonRunning(); | ||
| const pid = deps.readPid(); | ||
| if (!running && pid === null) { | ||
| deps.io.out("nectar is not running; nothing to stop."); | ||
| return 0; | ||
| } | ||
| const viaService = await deps.service.stop(); | ||
| if (viaService.ok) deps.io.out(viaService.message); | ||
| let signalled = false; | ||
| if (pid !== null) signalled = deps.sendSignal(pid, "SIGTERM"); | ||
| if (signalled) deps.io.out(`Sent SIGTERM to the nectar daemon (pid ${pid}).`); | ||
| if (!viaService.ok && !signalled) { | ||
| deps.io.out("nectar stop: no running daemon was signalled (it may already be stopped)."); | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Misleading "may already be stopped" message when the daemon is known to be running.
If isDaemonRunning() returned true but pid is null and viaService.stop() fails, the code prints "nectar stop: no running daemon was signalled (it may already be stopped)." — but running already confirmed the daemon IS alive; nothing here actually stopped it. The exit code staying 0 is intentional per the AC-9 docstring, but the message contradicts the known state and could mask a real failure to stop.
🛠️ Proposed fix: distinguish "confirmed running but couldn't stop it" from "nothing was ever running"
- if (!viaService.ok && !signalled) {
- deps.io.out("nectar stop: no running daemon was signalled (it may already be stopped).");
- }
+ if (!viaService.ok && !signalled) {
+ if (running) {
+ deps.io.err("nectar stop: the daemon is running but could not be stopped (service stop failed and no pid to signal).");
+ } else {
+ deps.io.out("nectar stop: no running daemon was signalled (it may already be stopped).");
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function runStopLifecycle(deps: StopLifecycleDeps): Promise<number> { | |
| const running = deps.isDaemonRunning(); | |
| const pid = deps.readPid(); | |
| if (!running && pid === null) { | |
| deps.io.out("nectar is not running; nothing to stop."); | |
| return 0; | |
| } | |
| const viaService = await deps.service.stop(); | |
| if (viaService.ok) deps.io.out(viaService.message); | |
| let signalled = false; | |
| if (pid !== null) signalled = deps.sendSignal(pid, "SIGTERM"); | |
| if (signalled) deps.io.out(`Sent SIGTERM to the nectar daemon (pid ${pid}).`); | |
| if (!viaService.ok && !signalled) { | |
| deps.io.out("nectar stop: no running daemon was signalled (it may already be stopped)."); | |
| } | |
| return 0; | |
| } | |
| export async function runStopLifecycle(deps: StopLifecycleDeps): Promise<number> { | |
| const running = deps.isDaemonRunning(); | |
| const pid = deps.readPid(); | |
| if (!running && pid === null) { | |
| deps.io.out("nectar is not running; nothing to stop."); | |
| return 0; | |
| } | |
| const viaService = await deps.service.stop(); | |
| if (viaService.ok) deps.io.out(viaService.message); | |
| let signalled = false; | |
| if (pid !== null) signalled = deps.sendSignal(pid, "SIGTERM"); | |
| if (signalled) deps.io.out(`Sent SIGTERM to the nectar daemon (pid ${pid}).`); | |
| if (!viaService.ok && !signalled) { | |
| if (running) { | |
| deps.io.err("nectar stop: the daemon is running but could not be stopped (service stop failed and no pid to signal)."); | |
| } else { | |
| deps.io.out("nectar stop: no running daemon was signalled (it may already be stopped)."); | |
| } | |
| } | |
| return 0; | |
| } |
🤖 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 `@src/lifecycle.ts` around lines 101 - 117, The fallback message in
runStopLifecycle is misleading when isDaemonRunning() confirms the daemon is
alive but neither service.stop() nor sendSignal() succeeds. Update the logic in
runStopLifecycle to distinguish the “known running but could not stop” case from
the “nothing was running” case, and emit a message that reflects the confirmed
running state when running is true and pid is null. Keep the 0 exit code
behavior unchanged, but adjust the conditional around viaService, pid, and
signalled so the output matches the actual daemon state.
Comment on lines
+179
to
+188
| export async function runUninstallLifecycle(deps: UninstallLifecycleDeps): Promise<number> { | ||
| const io = deps.io; | ||
| let hardFailure = false; | ||
|
|
||
| // 1. Stop the daemon (so doctor never sees a registered-but-gone product mid-flight). | ||
| const daemonWasRunning = deps.isDaemonRunning(); | ||
| const pid = deps.readPid(); | ||
| await deps.service.stop(); | ||
| if (pid !== null) deps.sendSignal(pid, "SIGTERM"); | ||
| if (daemonWasRunning) io.out("Stopped the running nectar daemon."); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/lifecycle.ts ==\n'
sed -n '1,260p' src/lifecycle.ts | cat -n
printf '\n== test/lifecycle.test.ts ==\n'
sed -n '1,320p' test/lifecycle.test.ts | cat -n
printf '\n== Search for service.stop and sendSignal definitions/usages ==\n'
rg -n "service\.stop\(|sendSignal\(|isDaemonRunning\(|readPid\(" src testRepository: legioncodeinc/nectar
Length of output: 27576
Uninstall should not claim the daemon stopped when both stop attempts fail. runUninstallLifecycle() ignores the results of service.stop() and sendSignal(), then prints Stopped the running nectar daemon. whenever isDaemonRunning() was true. If both attempts fail, uninstall can continue removing the unit, registry entry, and state dir while the daemon is still running. Consider surfacing that failure instead of reporting success.
🤖 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 `@src/lifecycle.ts` around lines 179 - 188, `runUninstallLifecycle()` currently
reports the daemon as stopped based only on `isDaemonRunning()`, even if both
`service.stop()` and `sendSignal()` fail. Update the uninstall flow to check the
outcome of those stop attempts in `runUninstallLifecycle` and only print the
success message when at least one stop action succeeds; if both fail, set
`hardFailure` or otherwise surface the failure and avoid claiming the daemon
stopped while proceeding through uninstall.
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
Nectar's slice of superproject PRD-003:
nectar login: a new in-process, zero-dependency Deeplake device-flow client (src/auth/device-flow.ts): URL + user code printed before any browser-open attempt, https-only validation before opening, poll floors +slow_downhonored with a bounded cap, explicit tenancy selection (prompt or--org/--workspace, never a silentorgs[0]), and a credentials file byte-compatible with honeycomb'sDiskCredentials, written via an atomic 0600 temp+rename.src/fleet-detection.tsmirrors honeycomb's three-signal contract; a daemon without credentials now serves 503 degraded with a machine-readable reason and flips healthy without restart when credentials appear (new credentials watch);nectar installauto-opens the popup only when solo with no credentials.start/stopverbs and a three-partuninstall(unit currentcom.legioncode.nectar+ legacycom.hivenectar.daemonfamily, registry entry delete preserving other entries, symlink-safe state-dir removal) with honest exit codes: already-absent units are friendly no-ops, genuine removal failures exit nonzero naming the error.Verification
library/ledger/EXECUTION_LEDGER-fleet-lifecycle.md.library/qa/security/2026-07-04-security-audit-prd-003-fleet-lifecycle.md.library/qa/quality/2026-07-05-qa-report-prd-003-fleet-lifecycle.md.Part of the-apiary PRD-003 fleet ship (sibling PRs in honeycomb, doctor, hive, and the superproject).
Summary by CodeRabbit
New Features
Bug Fixes