PRD-020 + PRD-019: apiary state root and project-scoped brooding activation#21
Merged
Merged
Conversation
…oding PRD-020 (ADR-0003/0005): nectar state moves to ~/.apiary/nectar/ via the shared resolveApiaryRoot chain (absolute-only env roots), one-time idempotent migration with legacy fallbacks, service-unit adoption, and doctor-registry window writes with absolute advertised paths. PRD-019: the daemon is dormant by default and multi-root. It broods and watches only bound, brooding-enabled projects from the shared folder bindings, reconciled on the poll cadence and on toggle; adds the brooding on/off store (~/.apiary/nectar/projects.json), the GET/POST /api/hive-graph/projects endpoints, nectar projects/brooding CLI verbs, the pathological-root guard, a dependency-free gitignore parser for non-git roots (ReDoS-hardened), per-project projection pre-warm, and shared-ignore parity on every CLI discovery path. Security audit fixed 1 High (ReDoS) in place; QA passed both PRDs with all warnings remediated (716 non-live tests, 0 fail). Live Deeplake probes are environment-gated (fresh-workspace visibility lag; see the superproject ledger investigation note).
📝 WalkthroughWalkthroughThis PR adds fleet-root path resolution and migration, multi-root active-project brooding control with ChangesFleet root path resolution and migration
One-time state migration
Multi-root active projects and brooding control
Dependency-free gitignore fallback
Service installer APIARY_HOME pinning
Security audit and QA reports
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProjectsApi
participant BroodingState
participant ProjectSupervisor
participant HealthState
CLI->>ProjectsApi: POST /api/hive-graph/projects/brooding
ProjectsApi->>ProjectsApi: parseBroodingToggle(body)
ProjectsApi->>BroodingState: persistProjectBrooding/persistGlobalBrooding
ProjectsApi->>ProjectSupervisor: reconcile()
ProjectSupervisor->>HealthState: setActiveProjects(slice)
ProjectsApi-->>CLI: updated ProjectsView
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…index busy-loop, non-gitignore exclude discoverability
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (13)
test/gitignore.test.ts (1)
32-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding regression coverage for the escaped-negation and chained-
*cases flagged ingitignore.ts.The existing negation test (line 32-36) and the globstar ReDoS regression tests (line 44-92) are solid, but neither covers a leading
\!/\#escape (compileRule) nor a chained-single-*pathological pattern. Once those are fixed, tests likecompileGitignore(["\\!important"])and a chained-*timing test analogous to the existing globstar one would pin both fixes.🤖 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/gitignore.test.ts` around lines 32 - 92, Add regression tests in gitignore.test.ts for the missing escape and single-star cases mentioned in the review. Extend the existing compileGitignore/compileRule coverage by asserting that leading escaped literals like \! and \# are treated as normal filenames, not negation/comment markers, and add a bounded-time pathological pattern test for chained single-* behavior similar to the existing collapseGlobStars timing test. Use the existing test structure around compileGitignore and the security-regression block to keep the new cases close to the related logic.src/service/platform.ts (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
nonBlankhelper.This trims-and-blanks-to-
undefinedhelper duplicatesnonBlankalready defined insrc/apiary-root.ts(used there forAPIARY_HOME/XDG_STATE_HOME). Consider exporting the one fromapiary-root.tsand importing it here to avoid two independently-maintained copies of the same normalization logic.♻️ Suggested consolidation
-function nonBlank(value: string | undefined): string | undefined { - if (value === undefined) return undefined; - const trimmed = value.trim(); - if (trimmed === "") return undefined; - return trimmed; -} +import { nonBlank } from "./apiary-root.js";🤖 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/service/platform.ts` around lines 104 - 108, Duplicate trims-to-undefined helper: the local nonBlank in platform.ts repeats the normalization logic already defined in apiary-root.ts. Consolidate by exporting nonBlank from apiary-root.ts and importing it here, then update any call sites in the platform service to use the shared helper so APIARY_HOME/XDG_STATE_HOME normalization stays consistent in one place.src/hive-graph/active-projects.ts (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant/dead outer guard.
platform(Line 66) is always defined (options.platform ?? process.platformnever yieldsundefined), sooptions.home !== undefined || platform !== undefinedis alwaystrue; the innerhome !== undefinedcheck on Line 80 already does the real work. Harmless but confusing.♻️ Simplify
- // $HOME. - if (options.home !== undefined || platform !== undefined) { - const home = options.home; - if (home !== undefined && home.trim() !== "" && norm === normalizeForCompare(home, platform)) return true; - } + // $HOME. + if (options.home !== undefined && options.home.trim() !== "" && norm === normalizeForCompare(options.home, platform)) { + return true; + }🤖 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/hive-graph/active-projects.ts` around lines 78 - 81, Remove the redundant outer guard in active-projects logic: in the code that checks `options.home` and `platform` before comparing against `normalizeForCompare`, `platform` is always defined via the existing `options.platform ?? process.platform` assignment, so the `options.home !== undefined || platform !== undefined` condition is dead. Simplify the control flow by relying on the existing `home !== undefined` check inside that block and keep the comparison logic in the same `normalizeForCompare`/`norm` path.src/project-supervisor.ts (1)
85-129: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo bounded timeout on
ctx.start()/ctx.stop().Unlike
daemon.ts's own drain path (which bounds waits viaraceWithTimeout),reconcileNowandstopAllawaita project'sstart()/stop()with no timeout. A single hungRunningContext(e.g. a stalled durable-store flush inproject-context.ts'sstop()) stalls the serializedqueueindefinitely, blocking reconciliation — and shutdown — for every other project too.🤖 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/project-supervisor.ts` around lines 85 - 129, The reconciliation flow in reconcileNow and stopAll can hang forever because ctx.start() and ctx.stop() are awaited without any deadline. Update the ProjectSupervisor logic to wrap both lifecycle calls with the same bounded timeout approach used in daemon.ts (for example via raceWithTimeout), and make sure the timeout handling still reports failures through onError while preserving the serialized queue behavior.test/project-supervisor.test.ts (1)
39-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo coverage for a throwing
factory.Given the finding on
project-supervisor.ts(an unguardedfactory()call can wedge the reconcile queue), a test exercisingfactorythrowing synchronously would have caught the regression and should guard against it going forward.🤖 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/project-supervisor.test.ts` around lines 39 - 104, Add a regression test in ProjectSupervisor’s reconcile coverage for a synchronous throw from the factory callback. Extend the existing reconcile tests around ProjectSupervisor, reconcile, and fakeContext by using a factory that throws on context creation, then assert the reconcile call does not wedge the queue, surfaces the error through onError, and leaves existing contexts/state consistent for a subsequent reconcile.src/registration/project-context.ts (1)
216-225: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
bridge.hydrateis not guarded by aclosedcheck before it runs.If
stop()races in between context creation (line 191-215) and thisawait bridge.hydrate(tenancy)call, hydrate still executes even thoughclosedwas already set — theclosedcheck only happens afterward (Line 221), beforeservice.start(). In practice the supervisor's serialized reconcile queue andPollLoop.whenIdle()inActiveProjectsController.stop()should prevent concurrent start/stop on the same context, so this is a narrow, likely-unreachable race rather than an active bug today — worth a defensive check if that invariant ever changes.🤖 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/registration/project-context.ts` around lines 216 - 225, The hydrate path in project-context startup can still run after the context has been closed because the closed guard only happens after bridge.hydrate(tenancy). Add a defensive closed check in the startup flow around the bridge.hydrate call in the project-context logic so that hydration is skipped when stop() has already marked the context closed, and keep the existing post-hydrate closed check before service.start() as well.src/api/loopback-client.ts (1)
107-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
DaemonSearchErroris now a misnomer for the projects/brooding endpoints.
daemonJsonRequestis generic and shared byprojectsViaDaemon/setBroodingViaDaemon, but non-2xx/non-JSON failures still throwDaemonSearchError, a name coined for the search endpoint. Purely cosmetic today since callers don't branch on it, but worth a more generic name (e.g.DaemonHttpError) before more endpoints reuse this helper.🤖 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/api/loopback-client.ts` around lines 107 - 147, The shared daemonJsonRequest helper currently throws DaemonSearchError for non-2xx and non-JSON responses, but this path is used by projectsViaDaemon and setBroodingViaDaemon too. Rename the error type to a generic daemon HTTP error name (for example DaemonHttpError) and update the throw sites inside daemonJsonRequest to use the new class so the naming matches its broader use.src/cli.ts (1)
921-957: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
nectar brooding <on|off> --allhas no partial-failure handling.The
--allbranch (lines 936-943) issues onesetBroodingViaDaemonPOST per project sequentially inside thetry. If the daemon errors or times out partway through, the command exits via the outercatchhaving already toggled some projects but not others, with no summary of which succeeded — the user has no way to know the resulting state without re-runningnectar projects.♻️ Proposed fix: isolate per-project failures and report a summary
} else { // --all: set every currently-bound project to the requested state. const current = await projectsViaDaemon(target); view = current; - for (const p of current.projects) { - view = await setBroodingViaDaemon(target, { projectId: p.projectId, brooding: invocation.brooding }); - } + const failures: string[] = []; + for (const p of current.projects) { + try { + view = await setBroodingViaDaemon(target, { projectId: p.projectId, brooding: invocation.brooding }); + } catch (err: unknown) { + failures.push(`${p.projectId}: ${err instanceof Error ? err.message : String(err)}`); + } + } + if (failures.length > 0) { + process.stderr.write(`nectar brooding: failed to update ${failures.length} project(s):\n ${failures.join("\n ")}\n`); + } }🤖 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 921 - 957, The `runBroodingCommand` `--all` path currently aborts on the first `setBroodingViaDaemon` failure, leaving projects partially updated with no summary. Update the `invocation.kind === "all"` branch to handle each project independently, capturing successes and failures around `setBroodingViaDaemon`, then print a concise summary of which projectIds were updated and which failed before returning an appropriate exit code. Keep the existing `DaemonUnreachableError` handling in the outer `catch`, but make per-project errors in the `--all` flow non-fatal until the end.library/knowledge/private/architecture/ADR-0005-fleet-directory-ownership-and-neutral-state-root.md (2)
41-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame citation gap for the registry-path-string claim.
The claim that "nectar's entry builder already writes resolved absolute paths" is stated without a
path:linecitation, inconsistent with the document's own citation convention used in the References section.As per coding guidelines,
library/knowledge/**/*.md: "Ground every claim with a source citation that includes a file path and line range, and never paraphrase signatures."🤖 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/knowledge/private/architecture/ADR-0005-fleet-directory-ownership-and-neutral-state-root.md` around lines 41 - 42, The registry-path-string claim in the ADR is missing a required source citation, so update the “Registry path strings” paragraph to include a path:line reference for the statement that nectar’s entry builder writes resolved absolute paths. Use the same citation style already used elsewhere in the document, and anchor the citation to the relevant source symbol or section so the claim is grounded without paraphrasing.Source: Coding guidelines
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing source citations for the security-amendment claims.
The security amendment (line 30) asserts implementation facts ("Absoluteness is checked with
path.win32.isAbsolute") and audit provenance ("from the W3 security audits") without a file-path/line-range citation, unlike the References section elsewhere in this document which usespath:lineformat.As per coding guidelines,
library/knowledge/**/*.md: "Ground every claim with a source citation that includes a file path and line range, and never paraphrase signatures."📝 Suggested citation addition
-Security amendment (2026-07-04, from the W3 security audits): env roots are honored ONLY when absolute; ... Absoluteness is checked with `path.win32.isAbsolute` (a strict superset of the posix check) so a relative value is never mistaken for absolute on any host. +Security amendment (2026-07-04, from the W3 security audits): env roots are honored ONLY when absolute; ... Absoluteness is checked with `path.win32.isAbsolute` (`src/apiary-root.ts:33-35`), a strict superset of the posix check, so a relative value is never mistaken for absolute on any host.🤖 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/knowledge/private/architecture/ADR-0005-fleet-directory-ownership-and-neutral-state-root.md` around lines 22 - 31, The security amendment in ADR-0005 contains uncited implementation and provenance claims, so add a proper source citation in the same style used by the document’s References section. Update the amendment text near the mention of path.win32.isAbsolute and the W3 security audits to include a file-path and line-range citation that grounds those facts, using the relevant ADR/reference source so the claim is traceable without paraphrasing any signatures.Source: Coding guidelines
src/service/index.ts (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates
launchdLogDir's stateDir logic insrc/service/templates.ts.Same ternary as
launchdLogDir(minus the/logssuffix). Consider exporting a singleserviceStateDir/nectarStateRoothelper fromtemplates.ts(or a shared module) and having this file consume it, to avoid the two copies drifting apart.🤖 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/service/index.ts` around lines 42 - 45, The state directory logic is duplicated between serviceStateDir in src/service/index.ts and launchdLogDir in src/service/templates.ts, so update them to use a single shared helper instead of separate ternaries. Move or export a reusable nectarStateRoot/serviceStateDir helper from templates.ts (or a shared module), then have serviceStateDir consume it and append the needed suffix locally so both paths stay in sync.src/service/templates.ts (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
stateDircomputation vs.src/service/index.ts.The
apiaryHome !== undefined ? ... : ...ternary here is byte-for-byte duplicated inserviceStateDir()insrc/service/index.ts(lines 42-44). Since this is the load-bearing fleet-root resolution logic (per ADR-0005), having two independent copies risks one being updated without the other.♻️ Suggested consolidation
-export function launchdLogDir(plan: Pick<ServicePlan, "home" | "apiaryHome">): string { - const stateDir = plan.apiaryHome !== undefined ? `${plan.apiaryHome}/nectar` : `${plan.home}/.apiary/nectar`; - return `${stateDir}/logs`; -} +export function serviceStateDir(plan: Pick<ServicePlan, "home" | "apiaryHome">): string { + return plan.apiaryHome !== undefined ? `${plan.apiaryHome}/nectar` : `${plan.home}/.apiary/nectar`; +} + +export function launchdLogDir(plan: Pick<ServicePlan, "home" | "apiaryHome">): string { + return `${serviceStateDir(plan)}/logs`; +}Then have
src/service/index.tsimport and reuseserviceStateDirfrom here instead of redefining it locally.🤖 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/service/templates.ts` around lines 32 - 35, The `stateDir` resolution logic is duplicated between `launchdLogDir` and `serviceStateDir`, so consolidate it into a single source of truth. Keep the existing `launchdLogDir` helper in `src/service/templates.ts`, and update `src/service/index.ts` to import and reuse `serviceStateDir` from this module instead of redefining the same `apiaryHome !== undefined ? ... : ...` ternary locally.src/telemetry-usage/emit.ts (1)
274-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated context-resolution block in
emitUsageEventandrecordDaemonStart.Both functions independently compute the same
env/dir/version/useLegacyFallbacktuple:const env = deps.env ?? process.env; const dir = deps.dir ?? defaultStateDir(env); const version = deps.version ?? readPackageVersion(); const useLegacyFallback = deps.dir === undefined;Consider extracting a small
resolveEmitContext(deps)helper to avoid the two call sites drifting apart as the migration logic evolves further.Also applies to: 375-390
🤖 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/telemetry-usage/emit.ts` around lines 274 - 288, Both emitUsageEvent and recordDaemonStart duplicate the same context-resolution logic for env, dir, version, and useLegacyFallback, which risks drift as migration behavior changes. Extract this repeated block into a small helper such as resolveEmitContext(deps) and have both emitUsageEvent and recordDaemonStart call it, keeping the existing readPackageVersion, defaultStateDir, and deps.env/deps.dir fallback behavior centralized in one place.
🤖 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.md`:
- Line 5: The audit note in the scope line exposes a machine-specific absolute
working-tree path; replace it with a repo-relative placeholder or generic
workspace reference while keeping the branch and commit context. Update the
affected markdown entry in the security audit note so the `Scope` text no longer
contains the local `C:/Users/...` path, preserving the same meaning without
leaking environment details.
In
`@library/requirements/backlog/prd-019-project-scoped-brooding-activation/qa/2026-07-04-qa-report-prd-019c-hive-dashboard-surface.md`:
- Line 49: The markdown table row for c-AC-5 is being split into extra columns
because the example text includes an unescaped pipe. Update the report entry in
the QA markdown so the cell content no longer contains a raw table delimiter;
either reword the example in the same row or replace/remove the pipe within the
`hive-graph.tsx` reference text so the table renders correctly.
In
`@library/requirements/backlog/prd-019-project-scoped-brooding-activation/qa/prd-019-project-scoped-brooding-activation-qa.md`:
- Line 9: Update the ordering note to reflect the current repository state,
since `library/qa/security/2026-07-04-security-audit.md` now exists. Edit the
note in `prd-019-project-scoped-brooding-activation-qa.md` so it no longer says
no 2026-07-04 security report file exists under nectar, while preserving the
rest of the ordering context about `security-worker-bee`, `collapseGlobStars`,
and `brooding-state.ts`.
In
`@library/requirements/backlog/prd-020-apiary-state-root-migration/qa/prd-020-apiary-state-root-migration-qa.md`:
- Line 9: Update the ordering note in the QA markdown to reflect the current
tree: the statement about no 2026-07-04 security report under nectar is stale
because `library/qa/security/2026-07-04-security-audit.md` now exists. Revise
that observation in the note to acknowledge the security audit file in the
branch while keeping the rest of the ordering context about
`security-worker-bee`, `src/apiary-root.ts`, `test/apiary-root.test.ts`,
`src/state-migration.ts`, and `src/registration/brooding-state.ts` intact.
In `@src/apiary-root.ts`:
- Around line 25-61: The root resolution logic in
resolveApiaryRoot/isAbsoluteRoot is treating Windows-style paths as absolute on
non-Windows hosts. Update the absolute-path validation to be platform-aware
before accepting APIARY_HOME or XDG_STATE_HOME, so win32.isAbsolute() is only
used when resolving for Windows (or otherwise reject drive-letter/UNC paths
outside Windows) and prevent nectarStateDir from inheriting a cwd-relative root.
In `@src/cli.ts`:
- Around line 1069-1092: The projects/brooding control surface in `src/cli.ts`
is incorrectly gated on `creds !== undefined`, which hides `nectar projects` and
`nectar brooding` even though `readActiveProjects`, `persistProjectBrooding`,
and `persistGlobalBrooding` only depend on the shared JSON state. Update the
`mountProjectsApi` wiring so it is attached unconditionally, while keeping the
active-project supervisor/setup (`activeProjects`, `createProjectContext`, and
related tenancy/store wiring) behind the existing creds/store checks. Also,
avoid calling `readActiveProjects(controlOptions)` twice on startup by resolving
it once and reusing that snapshot for both `primary` and `primaryTenancy`.
In `@src/daemon.ts`:
- Around line 999-1007: The shutdown path around activeProjectsController.stop()
is swallowing failures and leaving running contexts alive. In src/daemon.ts,
update the try/catch around activeProjectsController.stop() to add a defensive
fallback when stop() throws: use the activeProjectsController and its supervisor
to access supervisor.contexts() and force-stop or drain each remaining context
before shutdown completes. Keep the existing logging, but ensure the teardown
still fully stops per-project watchers/bridges even if stop() fails.
In `@src/doctor-registry.ts`:
- Around line 111-116: Create the fleet root before doctor registration:
`runInstall()` currently calls `registerWithDoctor({ config })` before
`~/.apiary` exists, so `defaultDoctorRegistryPath()` can still fall back to the
legacy `legacyRuntimeDir()` path. Update the install flow to ensure the root
from `resolveApiaryRoot()` is created first, or pass the fleet-root registry
path explicitly into `registerWithDoctor` so `doctor-registry` resolves to
`registry.json` instead of `doctor.daemons.json`.
In `@src/hive-graph/active-projects.ts`:
- Line 14: normalizeForCompare and isPathologicalRoot are still using host-bound
node:path behavior, so the injected platform is ignored when comparing or
checking roots. Update these helpers in active-projects.ts to choose path.win32
or path.posix based on the provided platform argument, and ensure all
parsing/resolve logic inside those helpers uses that platform-specific path
implementation so Windows roots like C:\ are handled correctly even on POSIX
hosts.
In `@src/project-supervisor.ts`:
- Around line 90-123: The reconcile flow in reconcileNow is letting a
synchronous failure from factory(project) escape, which can reject the shared
queue and block later reconcile/stopAll work. Wrap the ProjectContextFactory
creation in the same try/catch pattern used for ctx.start() and ctx.stop(), call
onError("start", project.projectId, err) for factory failures, and only add the
context to contextsById after successful creation so one bad project does not
poison future reconciles.
In `@src/registration/gitignore.ts`:
- Around line 104-112: The escaped prefix handling in gitignore parsing is being
reinterpreted as negation, so fix `src/registration/gitignore.ts` in the pattern
normalization flow before the `negate` check. Update the logic around the
leading `\#`/`\!` escape handling and the `line.startsWith("!")` branch so an
escaped `!` remains a literal pattern (`!foo`) and does not set `negate`, while
still preserving the existing behavior for unescaped negation. Keep the change
localized to the parser function that processes each line.
- Around line 46-92: The glob compilation in collapseGlobStars/globBodyToRegex
still leaves ReDoS risk because repeated single-star segments compile to
backtracking regex fragments. Replace the regex-based matching for gitignore
patterns with a linear-time glob matcher, or otherwise ensure `*` handling in
globBodyToRegex cannot backtrack on long non-matching paths. Keep the existing
`collapseGlobStars` behavior for `**` normalization, but update the matching
logic around `globBodyToRegex` to eliminate catastrophic backtracking for
patterns like chained `*a*a*...`.
In `@src/registration/ignore.ts`:
- Line 38: `ignore.ts` currently imports `DiskGitignore` only for local use, so
consumers cannot import it through this module. Update the `ignore.ts` module to
explicitly re-export `DiskGitignore` from `gitignore.js` (alongside the existing
`createDiskGitignore` usage) so external `import type { DiskGitignore } from
"../dist/registration/ignore.js"` works correctly.
- Around line 252-261: Pass the directory flag through the disk ignore check so
`DiskGitignore` can apply `build/`-style rules during descent. Update the
`walk()` logic in `src/registration/disk-fs.ts` to call the ignore predicate
with `isDir` for directory entries, matching the existing
`DiskGitignore`/`gitignoreFallback` contract. This should let ignored
directories be pruned before traversal instead of being treated as plain paths.
In `@src/service/templates.ts`:
- Around line 180-191: The cmd.exe wrapper built in templates.ts uses an unsafe
environment assignment pattern in the windowsCommand branch, so update the
wrapper in the code that constructs windowsCommand to use the canonical quoted
set syntax for APIARY_HOME instead of the current double-quoted form. Make sure
any values passed through the same wrapper, especially node and exec from
process.execPath and plan.execPath, are escaped with cmd-safe quoting so
metacharacters cannot leak into the task command.
In `@src/state-migration.ts`:
- Around line 132-164: The migration loop in state-migration treats a successful
copy plus a failed source cleanup as a full failure; split the
`migrateFile`/`copyThenRename` step from the `rmSync(sourcePath)` cleanup in
`src/state-migration.ts` so `moved` is recorded when the copy succeeds, while
delete errors are handled separately and retried later. Update the `migrateFile`
try/catch around `MIGRATION_RELATIVE_PATHS` to avoid marking the file as
`failed` just because source removal failed, and ensure orphaned legacy files
can still be cleaned up on a later boot. Add a `test/state-migration.test.ts`
case covering `migrateFile` success with a stubbed `rmSync` failure, asserting
the path is in `moved` not `failed` and that a subsequent run clears
`anyLegacyStateExists`.
- Around line 119-210: The state migration flow in runStateMigration can still
overlap across concurrent daemon starts because it runs before
acquireSingleInstanceLock, which leaves the legacy file moves and
registerWithDoctor registry refresh unprotected. Update the startup sequence so
the single-instance lock is acquired before invoking runStateMigration, or
otherwise serialize the shared registry update inside runStateMigration around
registerWithDoctor and the migration file moves. Use the existing
runStateMigration and registerWithDoctor symbols to keep the fix scoped to the
migration/registry refresh path.
In `@src/telemetry-usage/emit.ts`:
- Around line 206-212: The migration fallback in loadLedger currently has no
injectable legacy path seam, so tests can end up resolving against the real home
directory. Update loadLedger to accept and use a legacy runtime directory
dependency, mirroring readInstallId and resolveStateReadPath usage, and thread
deps.legacyRuntimeDir through the emitUsageEvent and recordDaemonStart call
sites when useLegacyFallback is enabled.
In `@test/project-context.test.ts`:
- Around line 87-125: The prewarm hermeticity test is missing the same
filesystem overrides used by the neighboring tests, so createProjectContext may
fall back to real createSharedIgnore and createDiskRegistrationFs behavior.
Update this test to pass the in-memory/shared-ignore overrides when constructing
createProjectContext, using the same registrationFs/sharedIgnore symbols as the
other hermetic cases, so the projection prewarm path does not touch real git or
disk machinery.
---
Nitpick comments:
In
`@library/knowledge/private/architecture/ADR-0005-fleet-directory-ownership-and-neutral-state-root.md`:
- Around line 41-42: The registry-path-string claim in the ADR is missing a
required source citation, so update the “Registry path strings” paragraph to
include a path:line reference for the statement that nectar’s entry builder
writes resolved absolute paths. Use the same citation style already used
elsewhere in the document, and anchor the citation to the relevant source symbol
or section so the claim is grounded without paraphrasing.
- Around line 22-31: The security amendment in ADR-0005 contains uncited
implementation and provenance claims, so add a proper source citation in the
same style used by the document’s References section. Update the amendment text
near the mention of path.win32.isAbsolute and the W3 security audits to include
a file-path and line-range citation that grounds those facts, using the relevant
ADR/reference source so the claim is traceable without paraphrasing any
signatures.
In `@src/api/loopback-client.ts`:
- Around line 107-147: The shared daemonJsonRequest helper currently throws
DaemonSearchError for non-2xx and non-JSON responses, but this path is used by
projectsViaDaemon and setBroodingViaDaemon too. Rename the error type to a
generic daemon HTTP error name (for example DaemonHttpError) and update the
throw sites inside daemonJsonRequest to use the new class so the naming matches
its broader use.
In `@src/cli.ts`:
- Around line 921-957: The `runBroodingCommand` `--all` path currently aborts on
the first `setBroodingViaDaemon` failure, leaving projects partially updated
with no summary. Update the `invocation.kind === "all"` branch to handle each
project independently, capturing successes and failures around
`setBroodingViaDaemon`, then print a concise summary of which projectIds were
updated and which failed before returning an appropriate exit code. Keep the
existing `DaemonUnreachableError` handling in the outer `catch`, but make
per-project errors in the `--all` flow non-fatal until the end.
In `@src/hive-graph/active-projects.ts`:
- Around line 78-81: Remove the redundant outer guard in active-projects logic:
in the code that checks `options.home` and `platform` before comparing against
`normalizeForCompare`, `platform` is always defined via the existing
`options.platform ?? process.platform` assignment, so the `options.home !==
undefined || platform !== undefined` condition is dead. Simplify the control
flow by relying on the existing `home !== undefined` check inside that block and
keep the comparison logic in the same `normalizeForCompare`/`norm` path.
In `@src/project-supervisor.ts`:
- Around line 85-129: The reconciliation flow in reconcileNow and stopAll can
hang forever because ctx.start() and ctx.stop() are awaited without any
deadline. Update the ProjectSupervisor logic to wrap both lifecycle calls with
the same bounded timeout approach used in daemon.ts (for example via
raceWithTimeout), and make sure the timeout handling still reports failures
through onError while preserving the serialized queue behavior.
In `@src/registration/project-context.ts`:
- Around line 216-225: The hydrate path in project-context startup can still run
after the context has been closed because the closed guard only happens after
bridge.hydrate(tenancy). Add a defensive closed check in the startup flow around
the bridge.hydrate call in the project-context logic so that hydration is
skipped when stop() has already marked the context closed, and keep the existing
post-hydrate closed check before service.start() as well.
In `@src/service/index.ts`:
- Around line 42-45: The state directory logic is duplicated between
serviceStateDir in src/service/index.ts and launchdLogDir in
src/service/templates.ts, so update them to use a single shared helper instead
of separate ternaries. Move or export a reusable nectarStateRoot/serviceStateDir
helper from templates.ts (or a shared module), then have serviceStateDir consume
it and append the needed suffix locally so both paths stay in sync.
In `@src/service/platform.ts`:
- Around line 104-108: Duplicate trims-to-undefined helper: the local nonBlank
in platform.ts repeats the normalization logic already defined in
apiary-root.ts. Consolidate by exporting nonBlank from apiary-root.ts and
importing it here, then update any call sites in the platform service to use the
shared helper so APIARY_HOME/XDG_STATE_HOME normalization stays consistent in
one place.
In `@src/service/templates.ts`:
- Around line 32-35: The `stateDir` resolution logic is duplicated between
`launchdLogDir` and `serviceStateDir`, so consolidate it into a single source of
truth. Keep the existing `launchdLogDir` helper in `src/service/templates.ts`,
and update `src/service/index.ts` to import and reuse `serviceStateDir` from
this module instead of redefining the same `apiaryHome !== undefined ? ... :
...` ternary locally.
In `@src/telemetry-usage/emit.ts`:
- Around line 274-288: Both emitUsageEvent and recordDaemonStart duplicate the
same context-resolution logic for env, dir, version, and useLegacyFallback,
which risks drift as migration behavior changes. Extract this repeated block
into a small helper such as resolveEmitContext(deps) and have both
emitUsageEvent and recordDaemonStart call it, keeping the existing
readPackageVersion, defaultStateDir, and deps.env/deps.dir fallback behavior
centralized in one place.
In `@test/gitignore.test.ts`:
- Around line 32-92: Add regression tests in gitignore.test.ts for the missing
escape and single-star cases mentioned in the review. Extend the existing
compileGitignore/compileRule coverage by asserting that leading escaped literals
like \! and \# are treated as normal filenames, not negation/comment markers,
and add a bounded-time pathological pattern test for chained single-* behavior
similar to the existing collapseGlobStars timing test. Use the existing test
structure around compileGitignore and the security-regression block to keep the
new cases close to the related logic.
In `@test/project-supervisor.test.ts`:
- Around line 39-104: Add a regression test in ProjectSupervisor’s reconcile
coverage for a synchronous throw from the factory callback. Extend the existing
reconcile tests around ProjectSupervisor, reconcile, and fakeContext by using a
factory that throws on context creation, then assert the reconcile call does not
wedge the queue, surfaces the error through onError, and leaves existing
contexts/state consistent for a subsequent reconcile.
🪄 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: 9ec2238b-4e37-4ea7-836b-057a32cd7762
📒 Files selected for processing (50)
library/knowledge/private/architecture/ADR-0005-fleet-directory-ownership-and-neutral-state-root.mdlibrary/qa/security/2026-07-04-security-audit.mdlibrary/requirements/backlog/prd-019-project-scoped-brooding-activation/qa/2026-07-04-qa-report-prd-019c-hive-dashboard-surface.mdlibrary/requirements/backlog/prd-019-project-scoped-brooding-activation/qa/prd-019-project-scoped-brooding-activation-qa.mdlibrary/requirements/backlog/prd-020-apiary-state-root-migration/qa/prd-020-apiary-state-root-migration-qa.mdsrc/active-projects-runtime.tssrc/api/loopback-client.tssrc/api/projects-api.tssrc/apiary-root.tssrc/cli.tssrc/config-file.tssrc/config.tssrc/daemon.tssrc/doctor-registry.tssrc/health.tssrc/hive-graph/active-projects.tssrc/index.tssrc/project-supervisor.tssrc/projects-control.tssrc/registration/brooding-state.tssrc/registration/disk-fs.tssrc/registration/gitignore.tssrc/registration/ignore.tssrc/registration/project-context.tssrc/service/argv.tssrc/service/index.tssrc/service/platform.tssrc/service/templates.tssrc/state-migration.tssrc/telemetry-usage/emit.tssrc/telemetry/db.tstest/active-projects.test.tstest/apiary-root.test.tstest/brooding-state.test.tstest/config.test.tstest/daemon-active-projects.test.tstest/doctor-registry.test.tstest/fixes.test.tstest/gitignore.test.tstest/project-context.test.tstest/project-supervisor.test.tstest/projects-api.test.tstest/projects-cli.test.tstest/service-argv.test.tstest/service-index.test.tstest/service-platform.test.tstest/service-templates.test.tstest/state-migration.test.tstest/telemetry-usage.test.tstest/telemetry/db.test.ts
… platform's path semantics isPathologicalRoot parsed paths with the HOST node:path module, so a win32-shaped fixture (C:\) evaluated on a POSIX CI host was not recognized as a drive root (posix parse of "C:\" has an empty root), failing the guard test on macOS/ubuntu while passing on Windows. The guard now selects path.win32 or path.posix from the injected platform, keeps $HOME comparison case-sensitive on POSIX (case-folded only on win32), and makes the WINDIR\System32 leg Windows-only so a POSIX directory that merely spells /Windows/System32 is never refused. Also defaults the $HOME guard to os.homedir() so the production daemon (which passes no explicit home) actually guards the real home directory per the PRD resolved decisions. Adds per-platform expectation tests covering both hosts.
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 `@test/active-projects.test.ts`:
- Line 129: The isPathologicalRoot test is using the host process.env.WINDIR
implicitly instead of a deterministic fixture, so make this assertion pass an
explicit env object that represents WINDIR unset. Update the test case in
active-projects.test.ts alongside the isPathologicalRoot call to use a
controlled env fixture, ensuring the win32 fallback behavior is validated
independently of the machine running the suite.
🪄 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: 239ea114-b013-4d9c-86b8-f3a9cbaed8b9
📒 Files selected for processing (2)
src/hive-graph/active-projects.tstest/active-projects.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hive-graph/active-projects.ts
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
Two PRDs on one branch (019 builds on 020's root helper).
PRD-020 (ADR-0003/0005):
~/.apiary/nectar/viaresolveApiaryRoot(absolute-only env roots, never cwd); pid/lock, nectar.json, pending-reviews, telemetry SQLite, usage-telemetry state all follow; one-time idempotent migration with ENOENT-gated legacy fallbacks and boot crash safety.APIARY_HOME; launchd logs under~/.apiary/nectar/logs; doctor-registry window writes with resolved absolute paths; fail-soft boot on a malformed registry (install verb stays fail-loud).PRD-019 (the original ingestion-scope fix):
/healthreports why.~/.deeplake/projects.jsonbindings, reconciled on the poll cadence and on toggle; pathological-root guard ($HOME, drive roots, System32).~/.apiary/nectar/projects.jsonstore,GET/POST /api/hive-graph/projects*(loopback, permission-gated, redacted errors),nectar projects/nectar broodingCLI verbs..gitignoreparser for non-git roots (ReDoS-hardened with bounded-time regression tests), shared predicate on every CLI discovery path, per-project projection pre-warm (PRD-011b rewired).Close-out (superproject ledger: library/ledger/EXECUTION_LEDGER-apiary-state-root.md)
library/qa/security/2026-07-04-security-audit.md.qa/folders. Hive-side 019c consumer verified field-for-field against the API.Test plan
Summary by CodeRabbit
/healthto include active, paused, and refused project details..gitignore/ignore fallback behavior and default ignoring during discovery.