Skip to content

PLA-53: one owner for connector construction - #115

Merged
hristo2612 merged 1 commit into
mainfrom
simplify/PLA-53-connector-single-owner
Aug 3, 2026
Merged

PLA-53: one owner for connector construction#115
hristo2612 merged 1 commit into
mainfrom
simplify/PLA-53-connector-single-owner

Conversation

@hristo2612

Copy link
Copy Markdown
Owner

Simplify Todo PLA-53. Base 23df25ed → head 800cdb62 (one commit).

Note: main on this remote is 75 commits behind the local main this branch was cut from, so the GitHub diff shows that backlog too. The PLA-53 change is the single commit 800cdb62. Same situation as #111/#112/#113/#114.

Selected area

Connector configuration and construction in packages/jinn/src/gateway/server.ts — the two parallel config paths (legacy top-level config.connectors.{slack,discord,telegram,whatsapp} vs config.connectors.instances[]) plus the connectorNames derivation feeding session context.

Evidence of over-engineering / blended concerns

The same construct/wire/start/register block was written three times (~260 of ~357 lines, L641-997):

  1. initConnector() helper at L665-703, used by the four top-level branches at L704-767.
  2. A 4-case switch re-implementing it inline in the boot instances[] loop, L772-867.
  3. A near-verbatim third copy of that switch inside reloadConnectorInstances(), L872-996.

The copies had drifted apart, and the drift was load-bearing:

  • Prose contradicted behaviour. initConnector's comment (L695-697) says start() must be fire-and-forget so "a slow handshake must not delay HTTP listen" — but the instances loop awaited discord.start() / slack.start() / whatsapp.start() / tg.start() during boot. An instance-configured connector with a slow handshake delayed HTTP listen; a top-level one did not.
  • Instance connectors were invisible to every employee prompt. connectorNames (L641-654) was built only from the four top-level keys, passed once into SessionManager (L658), stored at sessions/manager.ts:164, read at :434 to build the "## Available connectors" block (sessions/context.ts:395-399). So instances[]-declared connectors never appeared. Worse, SessionManager.setConfig() (:177) refreshed this.config but never this.connectorNames, so the cached copy went stale on reload.
  • The asymmetry leaked into the public API. reloadConnectorInstances skipped non-instance connectors via instanceConnectorIds (L888-889), so POST /api/connectors/reload (gateway/api.ts:6054-6067) silently no-opped for top-level connectors, and api.ts:6069/:6122 carried "supports both the legacy ... and named instance ids" special-casing.
  • The second config form was undocumented (template/docs/connectors.md, 72 lines, zero occurrences of "instances") and had no test anywhere in packages/jinn/src.

Fixed constraint budget

Frozen by the constrain phase before any code was written:

Field Budget
netLineDelta ≤ 0
maxFilesTouched ≤ 8
maxNewFiles ≤ 2
maxFileLines ≤ 7584

Plus: no new dependencies, no new config options, no new package-level public exports, no single-caller abstractions, no test deleted unless the behaviour it covered was deleted.

Measured budget (real output)

$ BASE=23df25ed62acfa162a39923638eb14dbbf1d8bea; git diff --numstat "$BASE" -- . ':!PLAN.md' | awk '{add+=$1; del+=$2} END {printf "netLineDelta=%d\n", add-del}'; echo "filesTouched=$(git diff --name-only "$BASE" -- . ':!PLAN.md' | wc -l | tr -d ' ')"; echo "newFiles=$(git diff --diff-filter=A --name-only "$BASE" -- . ':!PLAN.md' | wc -l | tr -d ' ')"; git diff --name-only "$BASE" -- . ':!PLAN.md' | { max=0; while IFS= read -r f; do if [ -f "$f" ]; then n=$(wc -l < "$f" | tr -d ' '); [ "$n" -gt "$max" ] && max=$n; fi; done; echo "maxFileLines=$max"; }

netLineDelta=-39
filesTouched=8
newFiles=1
maxFileLines=1452

Every number is inside budget. server.ts went 1646 → 1452 lines (−194). The only pre-existing file that grew is template/docs/connectors.md (+31, to 103 lines), which the budget exempted as documentation. The one new file is the test file.

What was deleted or clarified

Deleted — the duplication itself:

  • Both inline 4-case switches (boot loop + reloadConnectorInstances). Connector construction now exists in exactly one factory: grep -c 'case "telegram"' across server.ts totals 1.
  • instanceConnectorIds is gone entirely — grep -rn instanceConnectorIds packages/jinn/src returns nothing.
  • SessionManager no longer takes a connectorNames constructor param, so there is no cached copy left to go stale.

Clarified — one owner per concern:

  • One normalizer. Both config forms collapse into a single list: a top-level connector is just an instance whose id defaults to its type. Guards, duplicate ids, and missing ids are handled in one place.
  • One factory constructs every connector; one wiring function creates, routes, registers, and starts each one.
  • Boot fire-and-forgets start() for every connector, so the comment now matches behaviour on both paths and no handshake delays HTTP listen. (Reload still awaits, deliberately — callers want the result.)
  • Reload stops and restarts every connector regardless of which config form declared it. The { started, stopped, errors } response shape is unchanged.
  • Session-context connector names derive live from the connectorProvider registry, so instance-configured ids finally appear in employee prompts and reloads are reflected with no cache to refresh.
  • template/docs/connectors.md now documents instances[].

No new dependencies (package.json/lockfile untouched), no new config options (instances[] already existed at shared/types.ts:928), and both the factory and the normalizer replace ≥ 2 call sites each.

Test results

All gates run from the worktree at head 800cdb62:

  • pnpm typecheck — pass, 2/2 tasks.
  • pnpm test — pass: 310 test files, 3837 passed, 1 skipped, 0 failed.
  • pnpm build — pass, 2/2 tasks; web bundle emitted and synced to dist/web.

One new test file, packages/jinn/src/gateway/__tests__/connectors.test.ts (127 lines, 9 tests), covers normalization (legacy keys → id-defaulted instances, guards, duplicate/missing-id skip), factory dispatch, fire-and-forget boot (both never-resolving and rejecting start()), and connector-name derivation including a mixed legacy+instance config that reflects a reload. No existing test was deleted; the small edits to four existing test files are call-signature updates for the dropped SessionManager param.

Independent verify

Round-1 verify (separate session, read-only) returned ship — Blockers=0, Majors=0, Minors=3. The three non-blocking minors:

  1. The two fire-and-forget tests exercise a stub mirroring server.ts rather than the real boot loop.
  2. On reload, a connector whose start() rejects stays registered while its id lands only in errors[] — consistent with the legacy boot semantics.
  3. The legacy employee routing option is now captured at normalize time rather than message time (equivalent in practice).

Not verified: live handshakes against real connector credentials, and POST /api/connectors/reload against a running daemon — both covered only at the unit/normalization layer.

Connector setup was written three times: a legacy per-type block, an inline
4-case switch for `connectors.instances[]` at boot, and a near-verbatim copy of
that switch in reloadConnectorInstances(). The copies had drifted apart.

Both config forms now normalize into one list — a top-level connector is just an
instance whose id defaults to its type — and a single factory constructs every
connector. One wiring function creates, routes, registers, and starts each one.

- Boot fire-and-forgets start() for every connector, matching what the comment
  claimed; the instances path used to await each handshake before HTTP listen.
- Reload stops and restarts every connector, not just instance-declared ones;
  instanceConnectorIds is gone and the { started, stopped, errors } shape stays.
- Session context connector names come from the live connector registry, so
  instance-configured ids are listed and reloads are reflected without any
  cached copy to refresh (SessionManager no longer takes connectorNames).
- template/docs/connectors.md documents instances[].
@hristo2612
hristo2612 force-pushed the simplify/PLA-53-connector-single-owner branch from 800cdb6 to b68ce91 Compare August 3, 2026 09:01
@hristo2612
hristo2612 merged commit 8e3ae21 into main Aug 3, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant