Feature: github-install

[philcunliffe]

Auto-generated by /feature-launch to host the integration branch for feature github-install.

Work beads file individual sub-PRs into this branch via the refinery feature-flow loop. When all work + review + ship beads complete, the ship formula flips this PR to ready-for-review.

See /feature-flow for the flow architecture.

[philcunliffe]

Dual-agent review — request_changes

• Verdict: request_changes
• Risk class: high
• Blast-radius bead: hy-exum

Risk capstone

Cross-reference: reviewer findings that intersect high-risk surfaces

Source	Finding (severity, evidence)	Intersects
codex.md	Fix Validations / incomplete: readline interactive prompt path uses node:readline callback API as if promise-based (await rl.question(...)), so interactive confirmations can throw before returning a decision. Evidence: src/core/plugin_install/confirm.js:167, src/core/plugin_install/confirm.js:171, src/core/plugin_install/install.js:148.	Targets (confirm.js new, install.js modified); Concurrency surface §7 (confirm.js:168-179 readline buildTtyPrompt/ask — cited lines 167, 171 sit inside the same lifecycle)
codex.md	Behavioral Correctness / major / high: git ls-remote parsing takes the first SHA, which is wrong for annotated tags (refs/tags/x + refs/tags/x^{}), causing false "update available" against resolved_ref. Evidence: src/core/plugin_install/update_check.js:121, src/core/plugin_install/update_check.js:137, src/core/plugin_install/update_check.js:154.	Targets (update_check.js modified — +94 lines adding runGitProbe, execGit, parseLsRemoteOutput); same file as Concurrency surface §1 (update_check.js:180-202 execGit) — cited lines are in the new parseLsRemoteOutput helper that feeds the same probe path
codex.md	Security Surface / major / high: credential-bearing URLs can be persisted and echoed via source.raw (lock/confirmation), despite stderr redaction in git-error paths. Evidence: src/core/plugin_install/resolver.js:56, src/core/plugin_install/install.js:195, src/core/plugin_install/confirm.js:98, src/core/plugin_install/git_fetch.js:635.	Targets (resolver.js / install.js / confirm.js / git_fetch.js all in PR diff); Direct callers (resolver.js:53-69 parseGitSource caller block contains the cited resolver.js:56); Concurrency surface §1 (git_fetch.js:616-641 execGit decl contains the cited git_fetch.js:635)
claude.md	issue 1: git clone, git checkout, and git ls-remote pass user-controlled gitUrl / ref / target as positional args without a -- end-of-options separator (CVE-2018-17456 family); neither CLI parser nor parseGitSource/applyGitSourceFlags rejects leading -. Evidence: src/core/plugin_install/git_fetch.js:238-244, src/core/plugin_install/git_fetch.js:283-289, src/core/plugin_install/update_check.js:130-140.	Targets (git_fetch.js new, update_check.js modified); Concurrency surface §1 — every cited site is an execGit caller named in the surface map (runGitCloneSpan, runGitCheckoutSpan in git_fetch.js; runGitProbe in update_check.js)
claude.md	issue 2: post-install probe hangs indefinitely — after a successful git install/update, checkForPluginUpdate → runGitProbe → execGit(['ls-remote', ...]) resolves only on child 'close' with no timeout; a slow or blackholed remote hangs hyp plugin install after the artifact and lock are already written. Evidence: src/core/plugin_install/update_check.js:100-145.	Targets (update_check.js modified); Concurrency surface §1 (execGit "no timeout / no AbortSignal — a hung 'git ls-remote' never resolves"); Risks #2 — this finding is the exact failure mode the blast-radius report flags as MEDIUM ("execGit has no timeout... a hung git ls-remote (probe path) never resolves")
claude.md	issue 3: applyGitSourceFlags JSDoc says the subdir value is "still recorded on the returned spec so the eventual lock entry shape stays forward compatible", but the function throws git_subdir_unsupported unconditionally when opts.subdir !== undefined, so the trailing return { ...parts, ref, ...(subdir !== undefined ? { subdir } : {}) } is unreachable when subdir is set. Evidence: src/core/plugin_install/git_source.js:97-137.	Targets (git_source.js new — applyGitSourceFlags is the cited symbol); Direct callers (resolver.js:53-69 invokes parseGitSource/applyGitSourceFlags); Risks #6 — blast-radius report flags exactly this surface ("subdir is parsed but rejected... applyGitSourceFlags throws git_subdir_unsupported", a reserved-slot surface masquerading as a working flag)

Blast radius

• Lock-file RMW has no serializability and the install path has a
  multi-second double-write window. installPlugin writes the lock
  at line 220 (entry without update state), runs
  checkForPluginUpdate (which calls git ls-remote over the
  network — seconds to minutes), then writes again at line 238 using
  nextLock built from initialLock. Any concurrent hyp plugin install that lands between those writes is silently clobbered.
  Two concurrent installs of DIFFERENT plugins also lose one entry
  via plain last-writer-wins. No flock / lockfile / fcntl advisory
  lock anywhere on this path. Worth a follow-up bead to wrap the
  RMW in an in-process mutex + file lock, especially now that the
  fetch surface includes a network round-trip.
• execGit has no timeout and is duplicated. A hung
  git ls-remote (probe path) or git clone (install path) never
  resolves; the caller awaits forever and the child PID is
  reparented to PID 1 on hard exit. The implementation lives in
  BOTH update_check.js:180-202 and git_fetch.js:616-641, so any
  hardening (AbortController, signal, kill-on-cleanup) must land
  in two places or the binary is only half-hardened. Probe path is
  rate-limited to 24h so practical exposure is bounded; install
  path runs on every install.
• Daemon reload does NOT pick up newly installed plugins.
  daemon/runtime.js:reload() re-reads config and per-source
  reload() but does not re-call discoverInstalledPlugins or
  re-activate. The asymmetry is invisible: hyp status will show
  the newly installed plugin in the validation pass (because
  daemon/status.js re-discovers per-call), while the kernel's
  active-plugin set is frozen at boot. Operators following the
  documented install → reload ritual will get a misleading "all
  good" signal.
• Installed-plugin discovery failures are silently swallowed in
  three CLI paths. buildKnownPluginsForCtx wraps
  discoverInstalledPlugins in try / catch and returns an empty
  merged map on failure. daemon/status.js does the same. The
  underlying plugin.installed_manifest_invalid log row carries the
  detail, but the operator running hyp config validate sees
  "everything passed" with no signal that their installed plugins
  weren't considered. Worth a non-fatal warning printed to stderr
  when discovery returns failed[] entries.
• Shadow-collision is a hard boot fail with no graceful recovery.
  bootKernel throws installed_shadows_bundled if any installed
  plugin name matches a bundled one. The error message tells the
  operator to run hyp plugin remove, but the CLI can't reach the
  daemon if the daemon won't boot, and there is no "skip installed
  shadows and continue" flag. A user who installed
  @hypaware/local-fs from a fork and then ran hyp init ends up
  with a non-bootable kernel until they manually edit the lock file
  or rm -rf the install dir. Consistent with the Phase 7 design
  intent (hy-gh-2), but worth surfacing more loudly in the error
  message and the smoke surface.
• subdir is parsed but rejected — a "reserved" surface masquerading
  as a working flag. The --path CLI flag, the subdir resolver
  option, and the PluginSourceSpec.subdir typed field all exist and
  flow through the pipeline, but every code path that observes a
  non-empty subdir throws git_subdir_unsupported. Users hitting
  the CLI flag will get an error AFTER the clone has already happened.
  The clone is cheap, but the failure mode is "the path was reserved,
  it does not yet do anything" rather than "this flag is not yet
  implemented" — a --path flag that fast-fails in
  parsePluginInstallArgs would be a less surprising UX.
• Exit-code collision between usage error and confirmation
  required. runPluginInstall returns 2 for argv parse errors
  AND for remote_install_confirmation_required. CI scripts that
  treat exit 2 as "user typo, retry never" will misclassify a TTY
  detection failure (e.g., a CI runner with an unexpected stdin)
  as a permanent error. The stderr message distinguishes them; the
  exit code does not. Easy to split into 3 for confirm-required
  if scripts mature past hand-grep.

Codex review

Fix Validations

Remote install/update confirmation gate

• Status: incomplete
• Evidence: src/core/plugin_install/confirm.js:167, src/core/plugin_install/confirm.js:171, src/core/plugin_install/install.js:148
• Assessment: Non-TTY enforcement is wired, but the interactive prompt path uses node:readline callback API as if it were promise-based (await rl.question(...)), so interactive confirmations can throw before returning a decision.

Installed plugins included in config validation metadata

• Status: correct
• Evidence: src/core/cli/core_commands.js:831, src/core/cli/core_commands.js:1272, src/core/config/validate.js:153
• Assessment: Installed manifests are merged into known-plugin metadata, so installed names/capabilities now participate in validation instead of being treated as unknown by default.

Boot-time installed-vs-bundled shadow rejection

• Status: correct
• Evidence: src/core/runtime/boot.js:130, src/core/runtime/boot.js:149, src/core/runtime/boot.js:176
• Assessment: Collision detection runs before activation, emits explicit telemetry, and fails boot with installed_shadows_bundled.

Findings

[Behavioral Correctness]

• Severity: major
• Confidence: high
• Evidence: src/core/plugin_install/update_check.js:121, src/core/plugin_install/update_check.js:137, src/core/plugin_install/update_check.js:154
• Why it matters: git ls-remote parsing takes the first SHA, which is wrong for annotated tags (refs/tags/x + refs/tags/x^{}), causing false “update available” signals against resolved_ref.
• Suggested fix: Parse by refname and prefer the peeled ^{} commit SHA when present before comparing to entry.resolved_ref.

[Security Surface]

• Severity: major
• Confidence: high
• Evidence: src/core/plugin_install/resolver.js:56, src/core/plugin_install/install.js:195, src/core/plugin_install/confirm.js:98, src/core/plugin_install/git_fetch.js:635
• Why it matters: Credential-bearing URLs can be persisted and echoed via source.raw (lock/confirmation), despite stderr redaction in git-error paths.
• Suggested fix: Strip URL userinfo before persisting/displaying raw, store a redacted display form, and keep any sensitive clone URL ephemeral.

No Finding

• 2. Contract & Interface Fidelity
• 3. Change Impact / Blast Radius
• 4. Concurrency, Ordering & State Safety
• 5. Error Handling & Resilience
• 7. Resource Lifecycle & Cleanup
• 8. Release Safety
• 9. Test Evidence Quality
• 10. Architectural Consistency
• 11. Debuggability & Operability

Evidence Bundle

• Changed hot paths: src/core/cli/core_commands.js, src/core/plugin_install/install.js, src/core/plugin_install/git_fetch.js, src/core/plugin_install/update_check.js, src/core/runtime/boot.js, src/core/runtime/installed.js
• Impacted callers: src/core/cli/core_commands.js:832, src/core/cli/core_commands.js:987, src/core/plugin_install/fetch.js:75, src/core/plugin_install/install.js:273, src/core/daemon/status.js:257
• Impacted tests: test/core/plugin-install-confirm.test.js:18, test/core/plugin-install-git-source.test.js:14, test/core/boot-installed.test.js:88, hypaware-core/smoke/flows/plugin_install_git_url.js:1, hypaware-core/smoke/flows/plugin_install_github_url.js:1
• Unresolved uncertainty: interactive confirmation was not exercised in a real TTY test in this diff; annotated-tag upstream probe behavior is not covered by current tests/smokes.

Claude review

Code review

Note: This PR is a feature-flow integration branch (intentionally draft until ship). The code-review-quiet skill was invoked by mol-pr-dual-review with --no-comment --output, so the draft gate is overridden in this orchestrator context.

Found 3 issues:

1. Git option-injection: git clone, git checkout, and git ls-remote pass user-controlled gitUrl and ref/target as positional args without a -- end-of-options separator, and neither the CLI parser nor parseGitSource/applyGitSourceFlags rejects leading - in those values. A ref like --upload-pack=<cmd> (or a URL like --config=...) would be interpreted by git as an option rather than a value (CVE-2018-17456 family). Fix: insert -- before user-controlled positionals in each execGit call AND reject inputs matching ^- in the resolver / CLI parser.

hypaware/src/core/plugin_install/git_fetch.js

Lines 238 to 244 in 4f69e33

	git_repo: provenance.repo ?? '',
	},
	async (span) => {
	const cloned = await execGit(['clone', '--filter=blob:none', '--no-checkout', gitUrl, tmpRepo])
	if (cloned.code !== 0) {
	span.setAttribute('status', 'failed')
	span.setAttribute('error_kind', 'git_clone_failed')

hypaware/src/core/plugin_install/git_fetch.js

Lines 283 to 289 in 4f69e33

	message: 'plugin install: could not determine default branch on cloned repo',
	})
	}
	const checked = await execGit(['-C', tmpRepo, 'checkout', target])
	if (checked.code !== 0) {
	const isRefError = /pathspec|did not match|unknown revision|not a tree/i.test(checked.stderr)
	const errorKind = isRefError ? 'git_ref_not_found' : 'git_checkout_failed'

hypaware/src/core/plugin_install/update_check.js

Lines 130 to 140 in 4f69e33

	if (!gitUrl) {
	return { checked_at: checkedAt, available: false, error: 'missing gitUrl' }
	}
	const ref = entry.source.ref || 'HEAD'
	const probe = await execGit(['ls-remote', '--quiet', gitUrl, ref])
	if (probe.code !== 0) {
	return {
	checked_at: checkedAt,
	available: false,
	error: 'git_ls_remote_failed',
	}

2. Post-install probe can hang indefinitely: after a successful git install/update, the path awaits checkForPluginUpdate → runGitProbe → execGit(['ls-remote', '--quiet', gitUrl, ref]) inside the plugin.install span, and execGit resolves only on the child's close event with no timeout, so a slow or blackholed remote will hang hyp plugin install after the artifact and lock are already written. Fix: either skip checkForPluginUpdate for fresh git installs (the just-resolved resolved_ref is by definition current) or wrap the git ls-remote spawn with a hard timeout that emits a structured git_probe_timeout error_kind.

hypaware/src/core/plugin_install/update_check.js

Lines 100 to 145 in 4f69e33

	*/
	async function runProbe(entry, now) {
	const checkedAt = now.toISOString()
	if (entry.source.kind === 'local-dir') {
	return { checked_at: checkedAt, available: false }
	}
	if (entry.source.kind === 'git') {
	return runGitProbe(entry, checkedAt)
	}
	return {
	checked_at: checkedAt,
	available: false,
	}
	}
	/**
	* Probe a git source's upstream by running `git ls-remote`. The probe
	* compares the resolved upstream commit for the locked `source.ref`
	* (or the remote's default `HEAD`) against the entry's `resolved_ref`
	* and sets `available=true` when they differ.
	*
	* The probe is best-effort: a non-zero exit or unparseable output
	* records an error on the returned state but never throws.
	*
	* @param {import('../../../collectivus-plugin-kernel-types').PluginLockEntry} entry
	* @param {string} checkedAt
	* @returns {Promise<import('../../../collectivus-plugin-kernel-types').PluginUpdateState>}
	*/
	async function runGitProbe(entry, checkedAt) {
	const gitUrl = entry.source.gitUrl
	if (!gitUrl) {
	return { checked_at: checkedAt, available: false, error: 'missing gitUrl' }
	}
	const ref = entry.source.ref || 'HEAD'
	const probe = await execGit(['ls-remote', '--quiet', gitUrl, ref])
	if (probe.code !== 0) {
	return {
	checked_at: checkedAt,
	available: false,
	error: 'git_ls_remote_failed',
	}
	}
	const latestRef = parseLsRemoteOutput(probe.stdout)
	if (!latestRef) {
	return {
	checked_at: checkedAt,

3. applyGitSourceFlags JSDoc contradicts behavior: the docblock says the subdir value is "still recorded on the returned spec so the eventual lock entry shape stays forward compatible", but the function throws git_subdir_unsupported unconditionally when opts.subdir !== undefined, so the trailing return { ...parts, ref, ...(subdir !== undefined ? { subdir } : {}) } is unreachable when subdir is set and the documented forward-compatibility promise cannot be delivered. Fix: drop the "still recorded" sentence from the JSDoc, OR record the spec before throwing.

hypaware/src/core/plugin_install/git_source.js

Lines 97 to 137 in 4f69e33

	/**
	* Apply `--ref` / `--path` (subdir) CLI flags on top of a base
	* `GitSourceParts`. Enforces the two design-mandated rules:
	*
	* - Providing `--ref` AND a URL `#<ref>` fragment is ambiguous → throw
	* `source_ambiguous`.
	* - Providing `--path <subdir>` is unsupported in MVP → throw
	* `git_subdir_unsupported`, with the value still recorded on the
	* returned spec so the eventual lock entry shape stays forward
	* compatible.
	*
	* The caller decides how to wire the throw into telemetry; the kernel
	* funnels it through `resolveSource()`'s `resolver_error` path.
	*
	* @param {GitSourceParts} parts
	* @param {{ ref?: string, subdir?: string }} [opts]
	* @returns {GitSourceParts & { subdir?: string }}
	*/
	export function applyGitSourceFlags(parts, opts = {}) {
	let ref = parts.ref
	if (opts.ref) {
	if (parts.ref) {
	throw newGitSourceError(
	'source_ambiguous',
	`plugin install: --ref '${opts.ref}' conflicts with URL fragment '#${parts.ref}'`
	)
	}
	ref = opts.ref
	}
	const subdir = opts.subdir
	if (subdir !== undefined) {
	throw newGitSourceError(
	'git_subdir_unsupported',
	`plugin install: --path '${subdir}' is reserved but not yet supported`
	)
	}
	return { ...parts, ref, ...(subdir !== undefined ? { subdir } : {}) }
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

---

Reports: /Users/phil/testcity/.gc/pr-pipeline/reviews/pr-6 · Bead: hy-z8kt · Blast-radius: hy-exum