Skip to content

Add programmatic integration surface for embedding hosts - #144

Merged
philcunliffe merged 2 commits into
masterfrom
export-integration-points
Jun 25, 2026
Merged

Add programmatic integration surface for embedding hosts#144
philcunliffe merged 2 commits into
masterfrom
export-integration-points

Conversation

@platypii

Copy link
Copy Markdown
Contributor

Expose ./integration with run/attach/detach/join helpers that boot the kernel in-process and return structured results instead of spawning the hyp binary.

Expose ./integration with run/attach/detach/join helpers that boot the
kernel in-process and return structured results instead of spawning the
hyp binary.
@philcunliffe

Copy link
Copy Markdown
Contributor

Dual-agent review — request_changes

  • Verdict:request_changes
  • Risk class:medium
  • Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

SourceFinding (severity, evidence)Intersects
Codex #1major — join accepts dryRun but writes the seed anyway (integration.js:187,191; core_commands.js join has no --dry-run)Risks bullet 1 (published contract / unintended write). Orchestrator-confirmed against HEAD.
Codex #2major — attach('all')/detach('all') drops all-but-last ClientResult (integration.js:133,148; core_commands.js:3202,3213)Risks bullet 1 (published contract / silent data loss). Orchestrator-confirmed against HEAD.
Claudeminor — branch ~2 commits behind master; rebase before merge (package.json:3)Targets (package.json / exports). GitHub still reports MERGEABLE.
Claude (sub-threshold)minor — ClientResult omits real adapter fields (integration.d.ts:21-31)Risks bullet 2.
Claude (sub-threshold)minor — tests use a fake adapter; run().json + dryRun/noDaemon:false unexercised (integration.test.js)Risks bullet 3.
Codex review

Fix Validations

No bug-fix validation: PR context describes an additive integration surface, not a claimed failure-mode fix.

Findings

1) Behavioral Correctness

  • Severity: major
  • Confidence: high
  • Evidence: src/core/cli/integration.d.ts:8, src/core/cli/integration.js:187, src/core/cli/integration.js:191, src/core/cli/core_commands.js:3035
  • Why it matters:join(..., { dryRun: true }) is accepted by the public option type but still writes the central seed file because the helper never forwards or rejects dryRun.
  • Suggested fix: Either remove dryRun from the shared IntegrationOptions surface for helpers that cannot honor it, or make join reject opts.dryRun until runJoin has a real dry-run path; add a test proving dry-run does not create centralSeedPath.

2) Contract & Interface Fidelity

  • Severity: major
  • Confidence: high
  • Evidence: src/core/cli/integration.js:62, src/core/cli/integration.js:133, src/core/cli/integration.js:148, src/core/cli/core_commands.js:3212, src/core/cli/core_commands.js:3242, src/core/cli/core_commands.js:3373
  • Why it matters:attach('all') / detach('all') mutates every registered client, but the integration helper parses only the last JSON line and returns a single ClientResult, silently dropping the rest.
  • Suggested fix: Reject client === 'all' in the single-result helpers, or change the API to parse all JSON lines and return ClientResult[] for multi-client calls.

No Finding

  1. Change Impact / Blast Radius
  2. Concurrency, Ordering & State Safety
  3. Error Handling & Resilience
  4. Security Surface
  5. Resource Lifecycle & Cleanup
  6. Release Safety
  7. Test Evidence Quality
  8. Architectural Consistency
  9. Debuggability & Operability

Evidence Bundle

  • Changed hot paths: package.json:16; src/core/cli/integration.js:97; src/core/cli/integration.js:129; src/core/cli/integration.js:187
  • Impacted callers: package export for external consumers at package.json:16; in-repo test import at test/core/integration.test.js:9; test calls at test/core/integration.test.js:89, test/core/integration.test.js:105, test/core/integration.test.js:122, test/core/integration.test.js:138, test/core/integration.test.js:150, test/core/integration.test.js:160
  • Impacted tests: test/core/integration.test.js:87, test/core/integration.test.js:103, test/core/integration.test.js:117, test/core/integration.test.js:136, test/core/integration.test.js:147, test/core/integration.test.js:159
  • Unresolved uncertainty: I did not run tests; coverage currently does not exercise join(..., { dryRun: true }), attach('all') / detach('all'), or importing through the new ./integration package export.
Claude review

Claude review

Branch is ~2 commits behind master; rebase before merge

  • Severity: minor
  • Confidence: 85
  • Evidence: package.json:3 ("version": "1.6.1"); PR head sits on 80a2215 (Log claude model in backfill #134) while master tip is 105feba "Publish v1.7.0"
  • Why it matters: The branch predates the v1.7.0 publish, so the version line and the adjacent exports block are written against a stale base; GitHub still reports the PR MERGEABLE (no auto-merge conflict expected since the PR edits a different line of exports), but the merge would land 1.6.1 metadata unless rebased.
  • Suggested fix: Rebase onto current master and re-confirm the version and exports lines before merge. I verified the master-ahead commits (Recover Codex token counts for backfill and normalize token usage: net input + one carrier row per response #136 token-count normalization, v1.7.0 publish) touch neither dispatch, the seed/apply.js contract, the adapter --json shape, nor the exports map, so there is no hidden contract regression behind the staleness.

Verification notes (Claude reviewers + orchestrator): the five-lens Claude pass (guidance compliance, shallow bug scan, historical context, contract & callers, comments & tests) produced no blocker/major at ≥80 confidence. Sub-threshold items worth a glance, none gating:

  • parseLastJsonLine early-returns null on the first non-JSON final line instead of scanning earlier lines (conf ~25). Harmless for the curated attach/detach adapters (verified single-line JSON), but the name and the CommandResult.json JSDoc oversell the behavior for the generic run() escape hatch.
  • ClientResult (integration.d.ts) omits real adapter fields — codex config_path/base_url/restored_value, and removed/warning/error_kind on detach/failure paths (conf ~60). Typed consumers of hypaware/integration can't reach those through the documented surface.
  • join's declared token: string is required but the runtime and CLI both treat it as optional ([token], or --token-file) (conf ~50).
  • Tests drive a hand-rolled fakeClientKernel rather than the real claude/codex adapters; the single-line snake_case --json contract is duplicated in the fake, so adapter drift would not turn the suite red (conf ~55). dryRun/--dry-run and the noDaemon:false branch are unexercised (conf ~70).
  • New public surface restates documented decisions (daemon-not-in-process: LLP 0017; embedded join seed-only: LLP 0025/0031) in prose without an @ref, even though the wrapped CLI command carries those refs (conf ~50).

Orchestrator independently confirmed both Codex majors against current HEAD — see the cross-reference table in risk.md.


Reports: /Users/phil/workspace/hypaware/.git/worktrees/dual-review-pr-144/dual-review/pr-144

…ypes
Two correctness gaps in the new programmatic integration surface, both
confirmed by the dual-review, are baked into a published export and costly
to change once embedding hosts depend on it. Close them before merge:
- join({ dryRun: true }) no longer silently writes the central seed.
`hyp join` has no dry-run path; the helper forwarded neither a flag nor a
rejection, so a "preview" caller mutated state. join() now rejects dryRun
(and drops it from the typed options) — forward-compatible with a real
dry-run later. attach/detach still honor --dry-run as before.
- attach('all')/detach('all') no longer drop all-but-last result.
`hyp <verb> all` emits one JSON line per client; the single-result helper
kept only the last. runClient now rejects 'all' and points callers to a
per-client call or run(['attach','all','--json']) for the fan-out.
Folded-in review follow-ups while on this surface:
- parseLastJsonLine scans back past trailing non-JSON prose instead of
returning null on the first non-JSON final line; JSDoc/CommandResult.json
doc updated to match.
- ClientResult models the real adapter --json fields (config_path, base_url,
restored_value, removed, warning, error_kind) instead of a claude-only
subset; documented as the cross-adapter union.
- join's token is typed optional, matching the CLI ([token]/--token-file/stdin).
- @ref the seed-only-join (LLP 0025#seed-config-mode) and
daemon-not-in-process (LLP 0017#the-primary-daemon) decisions the surface
realizes.
Tests: dryRun-must-not-write, attach/detach 'all' rejected without touching
any client, run() multi-client escape hatch surfaces every client, and
run().json recovers JSON past trailing prose.
Kept additive on the PR head (no rebase/force-push); the change is orthogonal
to the v1.7.0 bump, so a GitHub merge preserves master's version.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor

Addressed the dual-review findings in 0e1adad (additive commit on the PR head — no rebase/force-push). All local gates green: npm test (1382 pass / 1 skip), tsc --noEmit, lint, and ref-check (2 refs, 0 broken).

Must-fix (the two majors)

  • join({ dryRun: true }) silently wrote the seedjoin() now rejectsdryRun (and it's dropped from the typed options via Omit<IntegrationOptions, 'dryRun'>), so a preview caller can never mutate state. Forward-compatible with a real dry-run later; attach/detach still honor --dry-run. Test: join({dryRun:true}) throws and asserts centralSeedPath was not created.
  • attach('all')/detach('all') dropped all-but-last resultrunClient now rejectsclient === 'all' (the single-result helper can't represent N clients) and points callers at a per-client call or run(['attach','all','--json']). Tests: both throw without touching any client, and a run() escape-hatch test confirms every client surfaces.

Should-fix (folded in)

  • parseLastJsonLine now scans back past trailing non-JSON prose instead of returning null on the first non-JSON final line; JSDoc + CommandResult.json doc corrected. New regression test recovers the JSON object past a trailing human line.
  • ClientResult now models the real adapter --json union (config_path, base_url, restored_value, removed, warning, error_kind) instead of a claude-only subset, documented with the source adapters.
  • join's token typed optional (token?: string), matching the CLI ([token] / --token-file / stdin).
  • @ref annotations added on join: LLP 0025#seed-config-mode (seed-only join) [implements] and LLP 0017#the-primary-daemon (daemon-not-in-process) [constrained-by].

Rebase note

Kept the fix additive on the PR head rather than rebasing onto v1.7.0 + force-pushing this branch. The change is orthogonal to the version bump (the PR doesn't touch version), so a GitHub merge preserves master's 1.7.0 — MERGEABLE confirmed. Happy to "Rebase and merge" server-side if you'd prefer the linear history.

Deliberately deferred

The "drive a real bundled adapter in a test" suggestion (sub-threshold/non-gating) is left for a follow-up: it needs the full ai-gateway + adapter activation path, which is heavier than this surface's hand-rolled fakeClientKernel. The new ClientResult keys now at least pin the cross-adapter wire contract the fake reproduces.

@philcunliffe
philcunliffe merged commit 3a075ea into masterJun 25, 2026
6 checks passed
@philcunliffe
philcunliffe deleted the export-integration-points branch June 25, 2026 19:30
Sign up for freeto 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.

2 participants

@platypii@philcunliffe