[Spec 5] feat: Interactive installer (ci-channel setup) — simpler rebuild#6
Merged
waleedkadous merged 8 commits intoApr 11, 2026
Merged
Conversation
Addresses iter1 feedback from Codex (REQUEST_CHANGES), Gemini (REQUEST_CHANGES), and Claude (COMMENT): - Embed canonical webhook + .mcp.json ci entry verbatim - Specify fetchSmeeChannel null → throw - Spell out single top-level try/catch error propagation - Endorse PATH-override for gh + globalThis.fetch stub as the no-DI test mocking strategy; clarify win32 skip - Rewrite contradictory Scenario 3 to 'state present, webhook missing → CREATE' (preserves state-first ordering test) - Tighten Scenario 2 assertion to byte-equal contents, keeping the unconditional write rule - Clarify --paginate --slurp returns array of pages (flatten) - Explicit webhook matching rule - Clarify .mcp.json creation when only .git/ exists - Add Consultation Log section
…stub, mcp.json 'add if missing' Four targeted corrections from the architect applied in-place before the spec-approval gate was approved. No second consult round (architect said 'no iter3'). 1. state.json: conditional write (deep-equal check vs disk), not 'always rewrite'. Correctness check, not a fast path. 2. Test mocking: drop globalThis.fetch stub. Tests now prepopulate state.json with fake smeeUrl + webhookSecret so fetchSmeeChannel is never invoked. That code path is covered by existing tests/bootstrap.test.ts. 3. .mcp.json: add mcpServers.ci if missing. If present, leave alone — respects user customizations. No deep-equal check. 4. Scenario 5 reworked: seed state with smeeUrl only (no secret) to force a write, then fail the POST call — the on-disk state must contain the fresh secret, proving state-first ordering.
Two-phase plan that respects the spec's hard constraints while satisfying porch's min-two-phases requirement via the single natural seam: implementation then tests. Phase 1: lib/setup.ts (≤150 lines) + server.ts dispatch (≤5 lines). Single file implementation with no DI, no separate modules. Includes an implementation sketch that projects ~134 lines with headroom, documents the deep-equal state write rule, the .mcp.json 'add if missing' rule, and the --paginate --slurp flatten. Manual smoke run as validation; existing 291 tests as regression gate. Phase 2: tests/setup.test.ts (≤200 lines, ≤8 tests) using the PATH-override fake-gh strategy and state.json prepopulation (no globalThis.fetch stub). Inline helpers, no separate helpers file. Scenarios 1-6 skipped on win32 (shell script fake); 7-8 are platform-independent.
Addresses Codex REQUEST_CHANGES + Claude APPROVE-with-nits from
iter1 plan review. Gemini approved with no issues.
From Codex:
- Expand Executive Summary to reconcile 'one-PR' spec rule
with porch's min-two-phases requirement
- State unchanged check: add Object.keys length === 2 to catch
extra-field cases (not just webhookSecret+smeeUrl match)
- .mcp.json: use 'ci' in servers (key presence) instead of
truthiness check — respects ci: null / ci: {} customizations
- Fake gh responses: add stderr field to { stdout, stderr, exitCode }
- Scenario 7: explicit finally cwd restore
From Claude:
- Fix git diff grep off-by-one (grep -c '^+[^+]')
- Remove hardcoded '291' baseline — record at Phase 1 start
- Remove deepEqual helper mention; show explicit 3-condition
boolean directly in sketch
- Move manual smoke from acceptance to commit-message-only
(requires live gh auth + disposable repo + smee.io)
Implements the 'ci-channel setup --repo owner/repo' subcommand
in a single file (lib/setup.ts, 120 lines) plus a 5-line
dispatch in server.ts. Covers all five install operations:
1. Load/generate webhook secret
2. Load/fetch smee.io channel URL
3. Write state.json (conditionally, deep-equal check) with
mode 0o600, BEFORE the webhook API call
4. List hooks via 'gh api --paginate --slurp' + flatten;
match on h.config.url === smeeUrl; PATCH if matching hook
found, else POST to create
5. Merge .mcp.json with key-presence check ('ci' in servers),
preserving user customizations
Error handling: single top-level try/catch inside setup();
all errors (findProjectRoot null, fetchSmeeChannel null, gh
non-zero exit, JSON.parse failures) flow through it and
become '[ci-channel] setup failed: <message>' to stderr +
process.exit(1).
Reviewed by Gemini (APPROVE), Codex (REQUEST_CHANGES then
fixed: blank-line removed so dispatch is exactly 5 added
lines + staged file concern addressed), Claude (APPROVE).
See codev/projects/5-spec-5-feat-interactive-instal/
5-impl-iter1-*.txt for review details.
Known trade-off: server.ts static imports (forges, bootstrap,
reconcile, smee-client) still evaluate in 'setup' mode. This
is acceptable under the ≤5-line dispatch cap; refactoring
those imports would blow the budget. The init side effects
of the imported modules are non-destructive; setup() exits
before they matter.
Test baseline: 173 tests on main, 173 tests after this
commit. No new tests in this phase — Phase 2 adds
tests/setup.test.ts.
Single test file (tests/setup.test.ts, 200 lines, 8 tests) covering the spec's 8 scenarios using the PATH-override fake gh strategy: 1. Fresh install with prepopulated state: POST + .mcp.json created + state byte-equal + mode 0o600 2. Idempotent re-run: PATCH once + state/.mcp.json byte-equal 3. State present, webhook missing: CREATE with existing secret 4. .mcp.json with other servers: ci added, other preserved 5. CREATE failure → state-first ordering (fresh secret on disk before failing POST attempt) 6. Project root discovered from subdirectory 7. No project root → exit 1 with 'project root' error 8. Missing --repo → exit 1 with usage message Test infrastructure: - PATH-override fake gh via POSIX shell script (skipped on win32 where shell scripts don't work out of the box) - Per-invocation logging to gh.args.N / gh.stdin.N files - Per-invocation canned responses via gh.out.N / gh.err.N / gh.exit.N files - mkdtempSync for real-fs test isolation - process.exit shim throws a sentinel that runSetup catches - stderr capture only; stdout left alone so node:test TAP output isn't swallowed (this bug bit me mid-iteration) - No globalThis.fetch stubbing — all tests prepopulate state.json so fetchSmeeChannel is never invoked; that path is covered by the existing tests/bootstrap.test.ts All 8 tests assert res.exitCode === null on success paths (Codex review: without this, tests could pass on silent failure-path exits). Scenario 1 asserts state.json byte equality to the seed (Codex review: spec requires this). Reviewed by Gemini (APPROVE), Codex (REQUEST_CHANGES twice on the same file, then APPROVE after fixes), Claude (APPROVE). All 181 tests pass (173 existing + 8 new).
Closes the review phase:
- codev/reviews/5-simpler-installer.md — comprehensive review
covering spec compliance, consultation feedback across 4
phases (spec, plan, impl, tests), architecture and lessons
learned updates, deviations (none), flaky tests (none), and
follow-up items (explicit non-goals).
- codev/resources/arch.md — directory tree now includes
lib/setup.ts, lib/project-root.ts, tests/setup.test.ts; new
Installer component section under Key Components; date
bumped to 2026-04-11.
- codev/resources/lessons-learned.md — four new entries:
1. Tight specs pay for themselves (Spec 3 → Spec 5 rebuild)
2. Never override process.stdout.write during node:test
execution (debug trap: tests silently disappear)
3. Natural-seam phase split for constrained single-PR specs
(impl vs tests)
4. Always assert exitCode === null on success paths when
process.exit is intercepted
Addresses Codex's PR review concern: parseArgs previously accepted args of any shape as long as --repo was present somewhere, silently ignoring unsupported flags like --forge, --smee-url, --dry-run, --yes, etc. This was a silent misconfiguration risk: a user typing 'ci-channel setup --repo foo/bar --forge gitlab' would get a GitHub webhook installed while believing they had configured a GitLab channel. Fix: parseArgs now iterates through all argv entries and throws on any token that isn't '--repo <value>'. The error message names the offending arg so the user can see what was rejected. This aligns with the spec's 'fail fast, no silent fallbacks' philosophy — the prior --repo regex rejection was about defensive parsing where gh produces clear errors anyway; this is different, because an unknown flag produces NO error downstream. Implementation: +3 lines in lib/setup.ts (120 → 123 → 125 with blank-line adjustments), well under the 150-line cap. Tests: expanded scenario 8 to cover both 'missing --repo' AND 'unknown flag rejected'. Same test count (8), +3 lines to tests/setup.test.ts. Net test file change stays under the 200-line cap (now 198) by compressing runSetup's try/catch/finally onto three lines (-5 lines).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rebuilds the
ci-channel setup --repo owner/reposubcommand as a tightly-constrained single-file installer. Supersedes #3 (Spec 3, PR #4 closed unmerged, 4,385 lines).Final size:
lib/setup.ts120 lines (cap 150),tests/setup.test.ts200 lines / 8 tests (cap 200/8),server.tsdispatch 5 lines (cap 5). No new dependencies, no new helper modules, no DI, no interactive prompts.Changes
lib/setup.ts(new, 120 lines) — the entire installer:parseArgs— extracts--repo owner/repo, throws usage error otherwiseghApi— promise wrapper aroundspawn('gh', …)with stdin piping and non-zero-exit → rejectsetup()— single try/catch covering all five ops: load state, generate/fetch missing fields, conditionally write state.json before the webhook call (state-first ordering), list hooks viagh api --paginate --slurp, PATCH the matching hook or CREATE a new one, addmcpServers.cito.mcp.jsonif missing (key-presence check, not truthiness — respects user customizations)server.ts(+5 lines) — dispatch block that checksprocess.argv[2] === 'setup'beforeloadConfig(); everything else passes through unchangedtests/setup.test.ts(new, 200 lines, 8 tests) — covers all 8 spec scenarios using PATH-override fakeghand state.json prepopulation (noglobalThis.fetchstubbing). Scenarios 1–6 skipped on win32 (shell scripts); 7–8 run everywhere.codev/specs/5-simpler-installer.md(updated) — spec iteration incorporated all 9 iter1 consultation concerns plus 4 architect correctionscodev/plans/5-simpler-installer.md(new) — two-phase plan (impl then tests) with the single natural seam that respects both the spec's "no phased implementation" rule and porch's min-two-phases requirementcodev/reviews/5-simpler-installer.md(new) — comprehensive review with consultation feedback from all 4 review roundscodev/resources/arch.md— new Installer component section + directory tree updatescodev/resources/lessons-learned.md— four new entries distilled from Spec 5 (tight specs, node:test stdout trap, natural-seam phase split, exitCode assertions)Design decisions worth knowing about
Object.keys(existing).length === 2). This is a correctness check (avoids mtime churn), not a speed optimization..mcp.jsonmerge:if (!('ci' in mcpServers)). A user who has added--reposto theircientry's args keeps their customization; the installer registers, it does not enforce.globalThis.fetchstub in tests: all tests prepopulate state.json sofetchSmeeChannelis never invoked. That path is covered by existingtests/bootstrap.test.ts.Testing
npm run buildpasseswc -l lib/setup.ts= 120,wc -l tests/setup.test.ts= 200,server.tsdiff = 5 added lineslib/setup.ts(grep -E 'InstallDeps|SetupError|UserDeclinedError|readline|@inquirer|confirm\(|prompt\('returns nothing)package.jsonReview gate checklist
Reviewers can run:
Consultation history
All four phases went through 3-way consultation (Gemini, Codex, Claude):
Full consultation feedback is documented in
codev/reviews/5-simpler-installer.md.Links
codev/specs/5-simpler-installer.mdcodev/plans/5-simpler-installer.mdcodev/reviews/5-simpler-installer.md🤖 Generated with Claude Code