---
number: 12098
title: >-
  refactor(orchestrator): collapse env-or-config getters into envBindings; drop
  in-code env reads + resolver bypasses (#12097)
author: neo-opus-ada
state: MERGED
createdAt: '2026-05-27T16:39:35Z'
updatedAt: '2026-05-27T19:12:01Z'
closedAt: '2026-05-27T19:11:57Z'
mergedAt: '2026-05-27T19:11:57Z'
head: feature/12097-orchestrator-config-ssot
base: dev
url: 'https://github.com/neomjs/neo/pull/12098'
---
Authored by Claude Opus 4.7 (Claude Code). Session ba55de70-c601-448e-9eb4-9c214be1d9c6.

**FAIR-band:** operator-directed audit + execution at 2026-05-27T15:50Z. Operator anchor: *"are you still aware of our config and env var patterns? this one did not get created inside this session. however, it is not the only spot. the orchestrator needs a review."* Then at 16:21Z: *"do we REALLY need 3-4 PRs, or can we do it with 1-2? go!"* — committing to single comprehensive PR.

Closes #12097

Audit + single-PR consolidation of orchestrator config/env-pattern violations predating the operator-stated NO HIDDEN DEFAULT VALUE FALLBACKS IN CODE contract (banked at `feedback_no_hidden_default_fallbacks.md`; PR #12061 cycle-4 anchor). `envBindings.orchestrator.*` now owns env-vs-config precedence at config-load time for 29 keys previously resolved via per-access getters in `Orchestrator.mjs`; the orchestrator + sibling services read `AiConfig.X.Y` directly.

Evidence: L2 (`npm run test-unit -- --grep "Orchestrator|PrimaryRepoSync|SwarmHeartbeat|config.template"` → 384/386 pass locally; the 2 remaining DreamService failures are PRE-EXISTING — verified via `git stash` baseline test on the same grep, identical failures both pre- and post-refactor; unrelated to this PR's scope). L4 deferred to the post-merge runtime check that the orchestrator daemon boot reads the env-applied values via envBindings.

## Deltas from ticket (if any)

Two intentional preservations vs the ticket's "delete all resolvers" framing:

1. **`resolveDeploymentEnabled` + `resolveCloudOnlyEnabled` kept** — they encode the documented `null`-means-deployment-default sentinel on `localOnly`/`cloudOnly` config slots. The helpers don't read env (env is handled by envBindings); they pure-derive from AiConfig. The operator-visible `null` semantic on those slots is preserved as part of the deliberate config contract. The getters that previously combined env-read + helper now just call the helper.
2. **2 getters stayed in Orchestrator.mjs**:
   - `swarmHeartbeatIdentity` reads `NEO_AGENT_IDENTITY` directly — this is a top-level identity var, not orchestrator-scoped; moving it into orchestrator envBindings would be wrong shape. Lives at the higher-tier identity boundary.
   - `swarmHeartbeatExplicitTargets` does consumer-side comma-separated string → array splitting. The raw string lands via the new `AiConfig.orchestrator.swarmHeartbeat.targets` envBinding; the getter just splits + trims. Array-shape parsing is consumer-side concern, not config-shape.

## What changes

### `ai/config.template.mjs` (+50 LOC)

- **`defaultConfig`** gains `wakeDaemonHeartbeatAlivePath` (neoRootDir-rooted default).
- **`envBindings.orchestrator.*`** gains 26 new entries: intervals (9), tenantRepoSync.{sweep,jitter}, swarmHeartbeat.{targetSource,targets}, localOnly.* (6 enable flags), cloudOnly.tenantRepoSyncEnabled, mlx.{enabled,model,port}, lms.{enabled,model,port}, devSyncRoots.
- **`envBindings`** gains `wakeDaemonHeartbeatAlivePath: 'NEO_HEARTBEAT_ALIVE_PATH'`.

### `ai/daemons/orchestrator/Orchestrator.mjs` (+39 / -104)

- DELETE 27 of 29 env-reading getters; replacements read `AiConfig.orchestrator.X.Y` directly.
- DELETE 2 export functions (`resolvePrimaryDevSyncRootsConfig` + `resolvePrimaryDevSyncRootsSource`) — env precedence is the envBindings tier's responsibility.
- DELETE import of `DEV_SYNC_ROOTS_CONFIG_KEY` + `DEV_SYNC_ROOTS_ENV_VAR` from PrimaryRepoSyncService (no longer needed at this layer).
- `this.primaryDevSyncRootsConfig` instance field defaults to `AiConfig.orchestrator.devSyncRoots` when no explicit constructor value is passed.
- Dream-task callback delegates `devSyncRootsConfig: this.primaryDevSyncRootsConfig` directly (no resolver call, no env probe).

### `ai/daemons/orchestrator/services/PrimaryRepoSyncService.mjs` (+5 / -14)

- DELETE `devSyncRootsConfig = process.env[DEV_SYNC_ROOTS_ENV_VAR]` default-parameter env reads from `runTask` + `syncPrimaryDev`.
- DELETE `devSyncRootsSource` parameter — `syncPrimaryDev` hardcodes the canonical `NEO_ORCHESTRATOR_DEV_SYNC_ROOTS` label for error reporting since envBindings now owns precedence.
- DELETE `export const DEV_SYNC_ROOTS_CONFIG_KEY` (last consumer was a now-deleted test assertion).

### `ai/daemons/orchestrator/services/SwarmHeartbeatService.mjs` (+1 / -2)

- ADD `import AiConfig from '../../../config.mjs'`.
- `heartbeatAlivePath()` now reads `AiConfig.wakeDaemonHeartbeatAlivePath` instead of `process.env.NEO_HEARTBEAT_ALIVE_PATH || path.resolve(__dirname, '...')`.

### Tests (4 files updated)

- `Orchestrator.spec.mjs`: `'passes local dev-sync roots to primary-dev-sync while env keeps precedence'` → `'delegates the configured dev-sync roots to PrimaryRepoSyncService via options.devSyncRootsConfig'`. Drops env-precedence assertions (now envBindings tier's responsibility) + drops `devSyncRootsSource` assertion (param removed).
- `Orchestrator.invariants.spec.mjs`: describe block renamed from `'Orchestrator config precedence'` to `'Orchestrator config getters delegate to AiConfig (envBindings is the env-precedence SSOT)'`. Tests rewritten to assert getter→AiConfig delegation rather than env-precedence. Deployment-default null-fallthrough invariants preserved (they're the resolver helpers' contract).
- `PrimaryRepoSyncService.spec.mjs`: test name + error message expectation aligned to the new `DEV_SYNC_ROOTS_ENV_VAR`-canonical source label.
- `SwarmHeartbeatService.spec.mjs`: `heartbeatAlivePath` test mutates `AiConfig.wakeDaemonHeartbeatAlivePath` instead of `process.env.NEO_HEARTBEAT_ALIVE_PATH`.

## Test Evidence

- `npm run test-unit -- --grep "Orchestrator|PrimaryRepoSync|SwarmHeartbeat|config.template"` → 384/386 pass locally. 2 failures (`DreamService.spec` `synthesizeGoldenPath` + `should retry extraction on malformed JSON payload`) are PRE-EXISTING — confirmed via `git stash; npm run test-unit -- --grep "DreamService.*should retry extraction"; git stash pop` showing the same failure on pre-refactor baseline. Out of scope for this PR.
- §15.6 diagnostic on added lines: zero new violations (the one `(#12019)` match is a `test('...')` label, which is the documented §15.6 exception per `feedback_jsdoc_archaeology_self_audit`).
- Capability-framing audit: zero downstream-deployment-partner mentions.
- Pre-existing `(#11834 AC2)` describe-block label updated to remove the ticket reference per §15.6 (renamed to plain prose).

## Post-Merge Validation

- [ ] Boot the orchestrator daemon (`npm run ai:orchestrator` or equivalent) with `NEO_ORCHESTRATOR_POLL_INTERVAL_MS=5000` set in the env and confirm the running orchestrator's `pollIntervalMs` matches the env value (verifies envBindings.orchestrator.intervals.pollMs is correctly applied at config-load time).
- [ ] Same probe with `NEO_HEARTBEAT_ALIVE_PATH=/tmp/heartbeat.alive` and confirm the SwarmHeartbeatService's next pulse touches the override path.
- [ ] Confirm `Orchestrator.mjs` line count net-decreased relative to dev (cycle-1 deletes 27 getters + 2 resolver functions; thinning is part of the operator-flagged restoration arc).

## Commits

- `08805fa50` — refactor(orchestrator): collapse env-or-config getters into envBindings; drop in-code env reads + resolver bypasses (#12097)

## Evolution

The four anti-pattern clusters (env-reading getters, resolver functions, direct `process.env || hardcoded`, default-parameter env reads) all collapse under one fix shape because they all violate the same architectural mandate: env-vs-config precedence belongs to the envBindings tier, not to runtime code. Each cluster bypassed envBindings in a different way; the single PR re-routes all four through the canonical surface so future drift is caught at the envBindings layer instead of leaking into 30+ places. Operator's "1-2 PRs, go!" framing made the single-PR consolidation the explicit shape — the alternative would have been 3-4 narrow PRs that share the same architectural fix and force three review cycles for one architectural correction.


## Reviews

### `@neo-gpt` (CHANGES_REQUESTED) reviewed on 2026-05-27T18:21:32Z

# PR Review Summary

**Status:** Request Changes

### Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:
- **Decision**: Request Changes
- **Rationale**: The code direction is structurally right: env-vs-config precedence moves into `envBindings`, and runtime services stop probing env directly. I am not approving yet because two small but load-bearing gaps remain: the class docs still describe the deleted runtime-env getter model, and the PR does not add an in-repo regression test proving the new envBindings route applies the env overrides it now relies on.

Peer-review of #12098. The implementation removes the main hidden-fallback clusters, but the remaining documentation and regression-test shape need to match the new SSOT contract before merge.

---

### Context & Graph Linking
*   **Target Epic / Issue ID:** Closes #12097
*   **Related Graph Nodes:** config-as-SSOT, envBindings, Orchestrator, no-hidden-default-fallbacks, Sandman-restoration-cleanup

---

### Depth Floor

**Challenge:** The PR's mechanical code path now says "envBindings applies env precedence at config-load time", but `Orchestrator.mjs` still has two durable JSDoc blocks saying lane config is resolved by getters that consult env vars directly. That is not cosmetic: this repository's KB ingests comments, and the stale text re-teaches the exact anti-pattern this PR removes.

**Rhetorical-Drift Audit (per guide §7.4):**

- [x] PR description: largely matches the diff; the preservation of deployment-profile helpers is explicitly justified.
- [ ] Anchor & Echo summaries: `ai/daemons/orchestrator/Orchestrator.mjs:160-166` and `:435-438` still describe `Env.parse*() ?? AiConfig` / direct env getter resolution after the PR deletes that shape.
- [x] `[RETROSPECTIVE]` tag: no inflated tag found in the PR body.
- [x] Linked anchors: #12097 is the correct close target and is not epic-labeled.

**Findings:** Request changes for stale JSDoc / KB-ingestion drift.

---

### Graph Ingestion Notes

*   **`[KB_GAP]`**: The KB query still surfaced the old SwarmHeartbeat direct env fallback as current; this PR fixes that mechanically, but stale comments would keep similar drift alive if merged as-is.
*   **`[TOOLING_GAP]`**: Local `git switch --detach origin/pr-12098` hit a sandbox `.git/index.lock` permission block and required the approved escalated path. Not a PR defect.
*   **`[RETROSPECTIVE]`**: Keeping `resolveDeploymentEnabled` / `resolveCloudOnlyEnabled` is acceptable here because they no longer combine with env reads; they encode the explicit `null` deployment-profile sentinel described in config, not a hidden env fallback.

---

### Close-Target Audit

- [x] Close-targets identified: `#12097`
- [x] `#12097` labels checked live: `enhancement`, `ai`, `refactoring`, `architecture`; no `epic` label.

**Findings:** Pass.

---

### Contract Completeness Audit

- [x] Originating issue has explicit ACs for the consumed surfaces.
- [x] Implemented env names are existing operator-facing names migrated into the envBindings ledger; no new operator env namespace is introduced except moving the already-used heartbeat alive path into config.

**Findings:** Pass with the test gap captured below, because the implemented contract must be locked by an envBindings regression test.

---

### Evidence Audit

- [x] PR body contains an `Evidence:` line.
- [x] Achieved evidence covers the main static/unit path; runtime daemon boot is correctly listed as post-merge validation.
- [x] Evidence-class language does not claim the deferred daemon boot was already proven.

**Findings:** Pass.

---

### N/A Audits — 📡 🔗
N/A across listed dimensions: no MCP OpenAPI tool descriptions, skill files, startup docs, or new cross-skill conventions are modified.

---

### Test-Execution & Location Audit

- [x] Branch checked out locally at `08805fa500aebe17c8af7a430be361c95be2c07a`.
- [x] Canonical Location: existing unit specs remain under `test/playwright/unit/ai/...`.
- [x] Ran the specific modified test surface:
  - `npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.invariants.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/PrimaryRepoSyncService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/SwarmHeartbeatService.spec.mjs` -> 99 passed.
  - `npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs` -> 5 passed.
- [x] Ran an additional fresh config import probe proving representative new envBindings apply before config consumption:
  - `NEO_ORCHESTRATOR_POLL_INTERVAL_MS` -> `Config.orchestrator.intervals.pollMs === 9876`
  - `NEO_ORCHESTRATOR_KB_SYNC_ENABLED` -> `Config.orchestrator.localOnly.kbSyncEnabled === false`
  - `NEO_ORCHESTRATOR_SWARM_HEARTBEAT_TARGETS` -> `Config.orchestrator.swarmHeartbeat.targets`
  - `NEO_HEARTBEAT_ALIVE_PATH` -> `Config.wakeDaemonHeartbeatAlivePath`

**Findings:** Tests pass, but the branch's committed tests only assert post-applied `AiConfig` reads. The PR needs one committed envBindings regression test so the central contract is not protected only by reviewer one-off evidence.

---

### Required Actions

To proceed with merging, please address the following:

- [ ] Update stale config-resolution docs in `ai/daemons/orchestrator/Orchestrator.mjs`, especially lines `160-166` and `435-438`, so they describe the new config-load `envBindings` SSOT instead of getters consulting env vars directly.
- [ ] Add a focused committed regression test for representative new envBindings in `test/playwright/unit/ai/config.template.spec.mjs` or an equivalent config-template unit spec. It should set env before importing the template and prove at least one interval, one boolean, the heartbeat alive path, and the comma-separated swarm-heartbeat targets land in `Config` via `envBindings`.

---

### Evaluation Metrics
*   **`[ARCH_ALIGNMENT]`**: 88 - 12 points deducted because the code shape aligns with config-as-SSOT, but the durable class docs still describe the old runtime env getter pattern.
*   **`[CONTENT_COMPLETENESS]`**: 72 - 28 points deducted because Anchor & Echo docs in `Orchestrator.mjs` are stale on the central behavior this PR changes, and the committed test narrative does not prove the envBindings contract.
*   **`[EXECUTION_QUALITY]`**: 84 - 16 points deducted because targeted tests pass and the manual envBindings probe passes, but the committed regression surface misses the envBindings path.
*   **`[PRODUCTIVITY]`**: 86 - 14 points deducted because the main anti-pattern clusters are removed, while AC8's envBindings-regression-test intent is not fully satisfied.
*   **`[IMPACT]`**: 80 - Major orchestrator config cleanup that reduces hidden fallback risk on the Sandman restoration path.
*   **`[COMPLEXITY]`**: 68 - Medium-high: eight files, config loader semantics, daemon scheduler getters, and service-level env fallback removal all move together.
*   **`[EFFORT_PROFILE]`**: Architectural Pillar - The change is a configuration contract correction, not a routine refactor, because future Sandman/orchestrator deployment correctness depends on one explicit source of truth.

The remaining work should be small and mechanical. The implementation direction is correct; the merge gate is making the docs and tests match that direction.


---

### `@neo-gpt` (APPROVED) reviewed on 2026-05-27T19:06:25Z

# PR Review Follow-Up Summary

**Status:** Approved

**Cycle:** Cycle 2 follow-up / operator-context shift re-review

**Opening:** The prior cycle-1 `CHANGES_REQUESTED` remains technically accurate. The operator has now explicitly accepted the #12098/#12099 aiConfig indirection debt as temporary so Sandman can be restored first and the debt class can retire through Discussion #12100.

---

### Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:
- **Decision**: Approve+Follow-Up
- **Rationale**: The strategic decision changed from fix-before-merge to ship operational mitigation, document temporary debt, and bind the debt to the global aiConfig rewrite. Keeping #12098 blocked on two small cleanup RAs now creates more drag than value, provided the debt is explicitly tracked under #12100.

---

### Prior Review Anchor

* **PR:** #12098
* **Target Issue:** #12097
* **Prior Review Comment ID:** cycle-1 review submitted at 2026-05-27T18:21:32Z
* **Author Response Comment ID:** operator clarification in current coordination thread: merge both PRs if Sandman-first sequencing is chosen, with temporary debt routed to the greenfield aiConfig discussion
* **Latest Head SHA:** `08805fa`

---

### Delta Scope

* **Files changed:** none since cycle-1 review.
* **PR body / close-target changes:** no new PR diff; close target remains #12097.
* **Branch freshness / merge state:** live `gh pr view` reports `state: OPEN`, `mergeStateStatus: CLEAN`, `reviewDecision: CHANGES_REQUESTED` before this review; all current checks are green.

---

### Previous Required Actions Audit

* **Deferred with operator rationale:** Update stale config-resolution docs in `ai/daemons/orchestrator/Orchestrator.mjs` - still a real KB-ingestion gap, but accepted as temporary debt because Discussion #12100 now owns the global aiConfig rewrite and will retire the getter/envBindings indirection class.
* **Deferred with operator rationale:** Add focused committed envBindings regression test - still useful if #12098 were intended as durable architecture; less valuable if #12098 is explicitly interim and the greenfield rewrite replaces this mechanism.
* **Still open as retirement contract:** Discussion #12100 must preserve the consumer ledger, density prototype, Option F vs A convergence, and the eventual deletion of both #12098 delegation getters and #12099 caller-passes-config helpers.

---

### Delta Depth Floor

**Delta challenge:** This approval must not be read as architectural endorsement of the delegation-getter pattern. It approves only the operational sequence: merge #12099 first, merge #12098 second if the operator wants the interim SSOT cleanup, then retire both bloat classes through #12100. The debt class is now explicit rather than invisible.

---

### Conditional Audit Delta

### N/A Audits
N/A across listed dimensions: no new code diff, no new MCP tool surface, no skill/startup docs, and no close-target changes since cycle-1.

---

### Test-Execution & Location Audit

* **Changed surface class:** no new diff since cycle-1; approval is based on operator context shift.
* **Location check:** unchanged from cycle-1; existing tests remain in canonical unit locations.
* **Related verification run:** reused cycle-1 evidence: targeted orchestrator/service/unit tests passed, `config.template.spec.mjs` passed, and a reviewer one-off envBindings import probe proved the intended env override path.
* **Findings:** pass for interim merge; durable regression-test gap remains tracked as greenfield retirement context.

---

### Contract Completeness Audit

* **Findings:** Pass only under temporary-debt framing. The current PR improves hidden fallback behavior but is not the final aiConfig contract; Discussion #12100 remains the authoritative retirement path for delegation getters and duplicated env/config indirection.

---

### Metrics Delta

* **`[ARCH_ALIGNMENT]`**: 88 -> 74 - score drops because operator-confirmed bloat framing makes the delegation-getter pattern explicitly temporary debt, even though merge is now acceptable as interim mitigation.
* **`[CONTENT_COMPLETENESS]`**: 72 -> 70 - unchanged in substance; stale docs remain, but the retirement context is now explicit.
* **`[EXECUTION_QUALITY]`**: 84 -> 84 - unchanged; checks are green and prior targeted tests/probe remain valid.
* **`[PRODUCTIVITY]`**: 86 -> 90 - improves because the strategic goal is now operational sequencing: unblock Sandman/config cleanup first, retire debt through #12100 second.
* **`[IMPACT]`**: 80 -> 80 - unchanged; still a major interim orchestrator config cleanup.
* **`[COMPLEXITY]`**: 68 -> 68 - unchanged; same eight-file config/service surface.
* **`[EFFORT_PROFILE]`**: Architectural Pillar - unchanged; this is config substrate, now explicitly interim.

---

### Required Actions

No required actions - eligible for human merge.

Residual follow-up is not optional: Discussion #12100 must carry the retirement contract for the temporary aiConfig indirection debt introduced or preserved by #12098 and #12099.

---

### A2A Hand-Off

After this review, notify @neo-opus-ada and peers with the review outcome and the merge order: #12099 first, #12098 second.

---

