feat(space): promote L2 connector abstraction + register github connector [#2301]

[lsm]

Promotes the #2300 spike's throwaway Connector contract to production and replaces every hardcoded 'github' in the engine with a connector-registry lookup — github is now one registered connector, pr_ready's connector dependency resolves through the registry, and sandboxed hook-env credential injection is driven by each connector's auth surface. pr_ready execution is byte-identical (the adapter is at the permission/auth layer; P2 #2302 re-expresses execution as a preset), so the deployed coding workflows behave the same. Flag-guarded: default on, HYPERNEO_WORKFLOW_CONNECTORS=0 falls back to the legacy hardcoded paths.

Also extracts the deferred shared runtime/gh-lookup-helpers.ts (env builder + resolveGithubConfigDir + runGhJson + rate-limit probe) shared by pr-ready-validator and the github connector, deleting the spike-local gh-client.ts.

Note: the L3/L4 pieces (external-state-validator, predicate, presets) stay spike-gated for #2302; pr_merged is not wired here (it's a preset re-expression = P2). One heads-up — the 5-space shard has a pre-existing flaky test (completion-detector) where an un-stopped SpaceRuntime tick loop fires rehydrateExecutors against a partial spaceManager mock; it's unrelated to this change (I don't touch the tick loop) and passed on re-run, but may occasionally flake in CI.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d84e5ddb58

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (Zhipu)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu

Recommendation: REQUEST_CHANGES — 1× P1 (behavior / sandbox-isolation divergence), 2× P3 nits. (Independently corroborated by the codex connector bot, which flagged the same P1.)

The promotion is clean and well-layered. The Connector / ConnectorAuth contract stays domain-agnostic, production.ts is the single honest wiring point, and the engine / validation / executor all consult the registry with no new special-casing in the default path. The honesty test holds — the only 'github' literals remaining in the default path are the documented HYPERNEO_WORKFLOW_CONNECTORS=0 fallbacks; space-runtime.ts event routing and long-horizon-agent-templates.ts are correctly out of scope (different subsystems). Verified locally: lint, typecheck, knip, session-guards, db-schema-parity all pass; the connector/validation/hook-engine/pr-ready tests pass. (The check:test-quality failure is pre-existing on dev in provider-registry.test.ts, untouched by this PR.)

But the task's central hard constraint — "behavior-preserving adapter" — is violated in one concrete, security-relevant case.

P1 — GH_HOST / GH_CONFIG_DIR leak into sandboxed hooks that don't permit github

The legacy buildHookRestrictedEnv loop stripped every GITHUB_LOOKUP_ENV_KEYS entry when github was not permitted:

if (GITHUB_LOOKUP_ENV_KEYS.has(key)) {
  if (permitGithub) env[key] = value;
  continue;   // <- stripped regardless of permitGithub
}

The new unified loop only injects permitted keys (hook-executor.ts:248):

if (permittedConnectorEnvKeys.has(key)) { env[key] = value; continue; }

When github is not permitted, permittedConnectorEnvKeys is empty, so GH_HOST and GH_CONFIG_DIR — which don't match RESTRICTED_ENV_KEY_PATTERN (TOKEN/SECRET/…) nor any restricted prefix — fall through and are passed to the sandboxed script. The four TOKEN-bearing keys are still stripped by the pattern, so the existing GH_TOKEN canary passes, but it gives false confidence: it never exercises the two non-TOKEN keys.

I empirically reproduced this against the PR head (permittedExternalLookups: [], both env vars set):

Case	script sees
connectors ON, github not permitted	host=ghe.corp.example.com|dir=/Users/x/.config/gh ← leak
connectors OFF (HYPERNEO_WORKFLOW_CONNECTORS=0), github not permitted	same ← leak
connectors ON, github permitted (control)	passed through, GH_CONFIG_DIR overridden to resolved value ✓

The second row is the larger problem: the PR states "The legacy branch is kept verbatim as a rollback fallback", but the fallback shares this new loop and does not restore legacy stripping. So the rollback safety net doesn't actually roll back this behavior.

Blast radius is low — GH_HOST (a hostname) and GH_CONFIG_DIR (a filesystem path) are not credentials, and deployed coding workflows are unaffected (they permit github). But the sandboxed hook env is an explicit least-privilege boundary, and the task's hard constraint is byte-identical behavior; any current/future non-github script hook gets a less-restricted env than before.

Suggested fix — restore "connector auth keys are scoped" semantics: strip a key when it belongs to any registered connector's auth.envKeys but that connector isn't permitted. Have resolvePermittedConnectorAuth also return the union of all registered connectors' auth keys, and in the loop:

if (permittedConnectorEnvKeys.has(key)) { env[key] = value; continue; }
if (allRegisteredConnectorAuthKeys.has(key)) continue;   // strip non-permitted connector keys

The legacy fallback branch should populate that strip-set from GITHUB_LOOKUP_ENV_KEYS so =0 is actually verbatim. Then extend the canary to assert GH_HOST / GH_CONFIG_DIR are absent when github isn't permitted (the current canary only proves GH_TOKEN is injected when permitted — it can't catch this regression).

P3 nits (non-blocking)

• connectors/presets.ts references the connector by the literal 'github' (3×) instead of GITHUB_CONNECTOR_ID. Spike-gated, so non-blocking, but the constant exists for exactly this.
• The global mutable registry + manual clearConnectorRegistry() / re-seed in afterEach/beforeAll works (tests pass) but is fragile under future reordering; a snapshot/restore helper would be more robust.

[lsm]

🤖 Review by glm-5.1 (Zhipu)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu

Recommendation: APPROVE (re-review of ee34de1a5).

P1 fixed and independently verified. resolvePermittedConnectorAuth now returns a managed superset (union of all registered connectors' auth.envKeys) alongside permitted; the env loop strips any managed key whose connector isn't permitted, restoring deny-by-default. The HYPERNEO_WORKFLOW_CONNECTORS=0 fallback sources managed from GITHUB_LOOKUP_ENV_KEYS, so it is verbatim again. I re-ran my repro against the fixed code:

• connectors ON, github not permitted → host=|dir= (both stripped) ✓
• connectors OFF (=0), github not permitted → host=|dir= (verbatim) ✓
• connectors ON, github permitted (control) → host passed through, GH_CONFIG_DIR override-resolved ✓

The new canary asserts missing|missing when denied (scriptEnv path intentionally left matching legacy — no behavior change there, correct). P3 nits addressed: presets.ts uses GITHUB_CONNECTOR_ID; production.test.ts snapshots/restores the registry around each test.

Verification: 5-space-runtime-a/b + 5-space-agent-other + Lint/Knip/Type + 1-core green in CI; 112 affected tests pass locally (engine, production, validation, pr-ready); both review threads resolved and not outdated. Remaining pending CI shards (4-space-migrations-a, online space-2/rewind-1, Greptile) don't touch this code. Zero remaining findings.

[greptile-apps]

Greptile Summary

This PR promotes the L2 connector abstraction from the #2300 spike into production, replacing every hardcoded 'github' string in the hook engine, executor, and validation layer with a generic registry lookup. The github connector is now one registered entry; pr_ready's connector dependency, sandbox credential injection, and externalLookups validation all resolve through the registry instead of special-casing the connector id.

Confidence Score: 5/5

Safe to merge — the connector abstraction is flag-guarded with a tested rollback path, sandbox env behavior is byte-identical to the legacy path, and the fetchRateLimitResetEpoch signature migration is applied consistently at every call site.

The change is a careful refactor with no functional changes to the pr_ready execution path. The managed/permitted env-key split is logically correct, the legacy fallback is preserved verbatim and exercised by tests, and both module-level Maps (connector registry + built-in deps) now have symmetric clear helpers.

Files Needing Attention: No files require special attention. The most load-bearing logic is in hook-executor.ts and gh-lookup-helpers.ts, both covered by the new canary tests.

Important Files Changed

Filename	Overview
packages/daemon/src/lib/space/runtime/connectors/production.ts	New file: seeds the connector registry and built-in deps map at module load; exports clearBuiltInConnectorDeps mirroring clearConnectorRegistry for test symmetry.
packages/daemon/src/lib/space/runtime/gh-lookup-helpers.ts	Extracted shared gh helpers; fetchRateLimitResetEpoch signature changed from absolute deadlineMs to relative timeoutMs — all callers correctly updated.
packages/daemon/src/lib/space/runtime/hook-executor.ts	resolvePermittedConnectorAuth replaces hardcoded permitGithub branch; managed/permitted split correctly handles non-TOKEN connector keys.
packages/daemon/src/lib/space/runtime/connectors/github-connector.ts	GITHUB_SANDBOX_ENV_KEYS matches the legacy hook-executor surface exactly; resolveExtraEnv provides path-verified GH_CONFIG_DIR override.
packages/daemon/src/lib/space/runtime/workflow-hook-engine.ts	permittedExternalLookups for built-in validators now reads from getBuiltInConnectorDeps registry; legacy branch preserved under flag guard.
packages/daemon/tests/unit/5-space/runtime/connectors/production.test.ts	New test file locks in the L2 contract: connector registration, auth.envKeys parity with legacy, and rollback behavior.

_{Reviews (2): Last reviewed commit: "refactor(space): address review nits — s..." | Re-trigger Greptile}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[lsm]

🤖 Review by glm-5.1 (Zhipu)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu

Recommendation: APPROVE — final pass at 99358aad (post dev-merge 33b3b435 + nit-fix commit).

The two remaining Greptile latent threads are addressed cleanly:

• production.ts exports clearBuiltInConnectorDeps() mirroring clearConnectorRegistry(); production.test.ts resets both in beforeAll — closes the registry/deps-map asymmetry.
• WorkflowHookExternalLookup = string & {} — nominal cue (honestly documented as non-enforcing); tsc --build --noEmit is clean across the workspace, so the brand breaks no consumers.

Verified at 99358aad: workspace typecheck clean; 106 affected tests pass (production / engine / validation / migration); the round-1 P1 deny-by-default fix in hook-executor.ts is untouched and intact; PR OPEN + MERGEABLE; all 4 review conversations resolved. CI green on 5-space-runtime-a; -b + Lint/Knip/Type freshly running on this commit (no failures). Zero findings.

(The third Greptile note — validation self-seeding via a side-effect import — was deliberately left as-is: all current production paths wire the registry correctly and the fail mode is fail-closed, so it's a safe deferral to #2302.)