Files

Kieran Klaassen 070092d997 feat(ce-polish-beta): human-in-the-loop polish phase between /ce:review and merge (#568 )

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

2026-04-16 17:55:10 -05:00

41 KiB

Raw Blame History

title: fix: Close ce-polish-beta detection gaps from PR #568 feedback type: fix status: active date: 2026-04-16

fix: Close ce-polish-beta detection gaps from PR #568 feedback

Overview

Address four concrete detection/resolution gaps in ce-polish-beta raised by @tmchow on EveryInc/compound-engineering-plugin#568:

Framework coverage — Nuxt, SvelteKit, Remix, Astro fall through to unknown (the commenter calls them "table stakes alongside Next and Vite")
Monorepo blind spot — detect-project-type.sh only inspects the repo root, so a Turborepo with apps/web/next.config.js returns unknown
Package-manager detection is documented in prose but not implemented; Next/Vite stubs silently write npm run dev on pnpm/yarn/bun projects
Port cascade is lossy — .env reader doesn't strip quotes or trailing comments, AGENTS.md/CLAUDE.md grep hits unrelated doc references, no probe of next.config.* / vite.config.* / config/puma.rb / docker-compose.yml

All four are detection/resolution bugs in an already-shipped beta skill (disable-model-invocation: true, so no auto-trigger regression risk). Fix scope is the skill's own scripts/ and references/ trees plus the Phase 3 wiring in SKILL.md.

Problem Frame

Polish's dev-server lifecycle (Phase 3 in SKILL.md) has three resolution jobs:

What project type is this? → scripts/detect-project-type.sh
How do I start it? → per-type recipe in references/dev-server-<type>.md, substituted into a launch.json stub
What port will it bind to? → inline cascade documented in references/dev-server-detection.md

All three jobs currently fail for common-but-unhandled shapes (monorepos, Nuxt/Astro, pnpm-only repos, quoted .env values). Users hit these gaps the first time they run polish on anything outside the four project types the skill was bootstrapped with (rails, next, vite, procfile). The fallback — "ask the user to author .claude/launch.json" — works but pushes onto the user a discovery problem the skill should do itself.

Feedback is the first real contact the skill has had with a reviewer outside the original plan, and it lines up with hazards already flagged in references/dev-server-vite.md ("SvelteKit, SolidStart, Qwik City, and Astro all use Vite… Different default ports apply") and references/dev-server-next.md ("Monorepo roots: users should set cwd… to the specific Next app"). The skill knew these were gaps and punted — this plan closes the punt.

Requirements Trace

R1. Nuxt, SvelteKit, Astro, and Remix are recognized first-class project types (no longer fall through to unknown).
R2. detect-project-type.sh finds a framework config inside a monorepo workspace (up to a bounded depth) and returns a type + relative cwd, so the stub-writer can populate cwd in launch.json without user intervention.
R3. Next and Vite stubs use the package manager indicated by the lockfile (pnpm / yarn / bun / npm) instead of hard-coding npm.
R4. Port resolution prefers authoritative config files (framework config, config/puma.rb, Procfile.dev, docker-compose.yml) over prose references. .env parsing correctly strips surrounding quotes and trailing # comment. The noisy AGENTS.md/CLAUDE.md grep is removed.
R5. Existing users are not regressed. Repos that previously detected correctly continue to detect the same type; repos with .claude/launch.json are unaffected (launch.json still wins).
R6. Each new or modified script has unit-test coverage in tests/skills/ mirroring the existing ce-polish-beta-dev-server.test.ts harness (tmp git repo, Bun.spawn, exit-code + stdout assertions).

Scope Boundaries

Not adding Python (Django, Flask, FastAPI), Go, Elixir/Phoenix, Deno/Fresh, Angular, Gatsby, Expo, Electron, Tauri, Storybook, or Ruby non-Rails (Sinatra, Hanami). Trevor listed these as gaps; they each need their own recipe file and dev-server conventions, and together they would roughly double the skill's surface area. Defer to a follow-up plan.
Not changing .claude/launch.json priority — launch.json always wins over auto-detect. This plan only improves what auto-detect does when launch.json is absent.
Not rewriting the IDE handoff, kill-by-port, or reachability probe in Phase 3.5/3.6. Those are unaffected.
Not changing headless-mode semantics. All new scripts are probes; they don't mutate state, so headless rules ("never write .claude/launch.json, never kill without token") are preserved.
Not adding a framework config parser beyond a conservative regex. Arbitrary JS/TS config files can set port via computed expressions the regex won't catch; when the probe misses, the cascade falls through to framework defaults. Document this as best-effort, not authoritative.
Not bumping plugin version, marketplace version, or writing a release entry. Per repo AGENTS.md, release-please owns that.

Context & Research

Relevant Code and Patterns

plugins/compound-engineering/skills/ce-polish-beta/scripts/detect-project-type.sh — current root-only classifier with precedence rules (rails beats procfile, multiple for real disambiguation)
plugins/compound-engineering/skills/ce-polish-beta/scripts/read-launch-json.sh — existing script that emits sentinel outputs (__NO_LAUNCH_JSON__, __INVALID_LAUNCH_JSON__, __MISSING_CONFIGURATIONS__, __CONFIG_NOT_FOUND__). The sentinel pattern is the convention new scripts should follow for signaling "no match, fall through"
plugins/compound-engineering/skills/ce-polish-beta/scripts/parse-checklist.sh — pattern for set-unsafe set -u, bash regex ([[ =~ ]]), and awk/jq composition within a single script. New scripts should match this style (no set -euo pipefail; the existing scripts use set -u only, by convention)
plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-<rails|next|vite|procfile>.md — per-type recipe shape: Signature, Start command, Port, Stub generation, Common gotchas
plugins/compound-engineering/skills/ce-polish-beta/references/launch-json-schema.md — stub templates grouped by project type; the stub-writer block to parameterize
tests/skills/ce-polish-beta-dev-server.test.ts — test harness pattern: tmp git repo, touch signature files, invoke script via Bun.spawn, assert exitCode + stdout.trim(). All new scripts follow this shape.
plugins/compound-engineering/skills/ce-polish-beta/SKILL.md Phase 3.2 (lines 272-291) — project-type routing table; the surface that needs extending for new types and the <type>@<cwd> return variant
plugins/compound-engineering/skills/ce-polish-beta/SKILL.md Phase 3.3 (lines 293-303) — stub-writer; where package-manager substitution and cwd population land

Institutional Learnings

None directly applicable; this work extends patterns already proven in the same skill.

Cross-Repo Reference (informational only)

plugins/compound-engineering/skills/test-browser/SKILL.md has an inline port cascade that polish's dev-server-detection.md is a copy of (per the self-contained-skill rule). This plan does not modify test-browser — the two cascades stay independent by design. Note for maintainers: if test-browser adopts a parallel resolve-port script later, the two skills will need the standard manual-sync note updated.

Key Technical Decisions

Decision: detect-project-type.sh returns <type> at root and <type>@<cwd> for monorepo hits, never just <cwd>. Rationale: keeps the existing single-token protocol intact for the 90% root-detection case; downstream readers split on @ when present. @ is chosen over : because : is reserved for the outer multi-hit separator (see below). Alternative considered: return structured JSON. Rejected because every other script in scripts/ returns plain-text tokens and consumers use case/awk on them, and JSON would force jq onto a detector that today only uses bash builtins.
Decision: Output grammar is <type> or <type>@<cwd> for single hits, multiple or multiple:<type>@<cwd>,<type>@<cwd>,... for multi-hits. The four concrete shapes are:
- next (single hit at root)
- next@apps/web (single hit in monorepo)
- multiple (multiple signatures at root — existing behavior, unchanged)
- multiple:next@apps/web,rails@apps/api (multiple hits across monorepo workspaces, always emitted as type@path pairs even when types are the same) Rationale: : is the outer multi-hit delimiter and @ is the inner type-path delimiter, making the grammar unambiguous under naive awk -F: or bash parameter expansion. Document this explicitly in the script header comment so callers cannot misread it.
Decision: New scripts accept an optional path as a positional argument, not --cwd. Rationale: every existing script in scripts/ uses positional args (parse-checklist.sh <path>, classify-oversized.sh <path> <path>) or derives cwd from git rev-parse --show-toplevel. Flag-parsing would be a new convention. Follow the existing pattern: optional positional path defaults to git rev-parse --show-toplevel.
Decision: Expected-no-result sentinels exit 0, not 1. Rationale: the existing convention in read-launch-json.sh (header comment on lines 20-21 of that file) reserves non-zero exit for operational failure only (missing jq, no git root). __NO_PACKAGE_JSON__ and similar sentinels exit 0 with the sentinel on stdout; callers pattern-match on stdout, not exit code.
Decision: No provenance output on stderr. Rationale: stderr across all existing scripts is reserved for ERROR: ... messages only. Provenance ("resolved_from: framework_config") would break that convention. resolve-port.sh emits a single-line integer on stdout, matching the simplicity of existing scripts. If future debugging surfaces real demand for provenance, add a second script or a --verbose mode in a follow-up — not speculatively.
Decision: Monorepo probe has a depth cap of 3 and walks only if root detection returned unknown. Rationale: depth 3 covers the common layouts (apps/web/next.config.js, packages/frontend/vite.config.ts, services/api/next.config.js). Running unconditionally would slow the common case and risk false positives when the root is a known type with example configs nested elsewhere (fixtures, templates). Depth 3 is a hard cap because deeper nesting usually means the user already needs to author launch.json.
Decision: Exclude node_modules/, .git/, vendor/, dist/, build/, coverage/, .next/, .nuxt/, .svelte-kit/, .turbo/, tmp/, fixtures/ from the monorepo probe. Rationale: these directories ship config files as fixtures or build output that the user doesn't own. Without exclusion, a Rails app with node_modules/next/.../examples/ would register as Next, and a monorepo with test fixtures would surface false positives.
Decision: resolve-package-manager.sh returns one token (npm / pnpm / yarn / bun) plus the start command (stdout line 1 and line 2 respectively) so stub-writer substitution is deterministic. Rationale: pnpm dev and bun run dev use different argv shapes. A single-token return would force the consumer to maintain a lookup table; emitting both the binary and the canonical args keeps all PM-specific knowledge in one place (the resolver).
Decision: resolve-port.sh replaces the inline dev-server-detection.md cascade. Rationale: the cascade lives in skill prose and has silently-buggy shell (unstripped quotes, noisy grep). Lifting it into a tested script with the sentinel-output convention makes the behavior assertable and fixes the bugs at the same site. dev-server-detection.md becomes a thin pointer to the script with the framework-default table retained.
Decision: Port cascade probes authoritative config files first, .env* second, default last. Rationale: Trevor's core complaint is that the current cascade prefers prose (AGENTS.md) over config (next.config.js, config/puma.rb). Flipping that ordering restores "the code is the source of truth."
Decision: Drop the AGENTS.md / CLAUDE.md grep entirely. Rationale: users who need to override have the explicit --port / port: CLI token and the .claude/launch.json escape hatch. Grepping instruction files for port numbers catches unrelated mentions ("connects to Stripe on port 8443", "example: localhost:3000") far more often than it captures a real override.
Decision: Framework config probes use a conservative regex and treat misses as "no pin, fall through". Rationale: parsing arbitrary JS/TS reliably requires a JS runtime, which polish doesn't ship with. A regex that catches port: 3000, port: "3000", and server: { port: 3000 } literals covers the common patterns. Missed ports fall through to framework default — same behavior as today, just with more chances to catch an explicit value along the way.

Open Questions

Resolved During Planning

Should Remix get a dedicated signature or route through Vite? Resolved: both. Classic Remix ships remix.config.js without Vite; Remix 2.x+ ships vite.config.ts. Classic pattern gets its own signature in the detector so it resolves without ambiguity; new Remix continues to resolve as vite (the existing Vite recipe already documents SvelteKit/Astro/etc. as framework-on-Vite). The remix recipe notes both paths.
Should the monorepo probe return all matches or just one? Resolved: return one if there's a single match, multiple with <type>@<path> pairs if several. Multiple matches at depth ≤3 is the genuine disambiguation case the existing multiple sentinel was designed for; the new output is multiple:next@apps/web,next@apps/admin so the interactive prompt in Phase 3.2 can list the options.
Where does SKILL.md document the new <type>@<cwd> format? Resolved: extend the existing Phase 3.2 routing table with a "Paths with @<cwd> suffix" paragraph and update Phase 3.3 to substitute cwd when present. No new top-level section.
Does the port resolver need to parse docker-compose.yml? Resolved: yes, but lightly — grep for - "<port>:<port>" under a ports: key on the service named web / app / frontend. Full YAML parsing is out of scope; a line-anchored regex catches the common compose shape and misses gracefully on exotic configs.

Deferred to Implementation

Exact regex for framework config port probes. Start with port:\s*[0-9]+ and port:\s*["']?[0-9]+["']?, tighten if tests surface false positives. Unit 4 owns this.
Whether pnpm dev should be pnpm dev or pnpm run dev. Both work; pick whichever is idiomatic per the current pnpm docs at the time of implementation and pin it in the resolver's lookup table.
Whether to probe bun.lock ahead of bun.lockb. Bun recently added a text lockfile format (bun.lock) alongside the binary (bun.lockb); priority likely doesn't matter (only one will be present) but the resolver should match whichever is there.

Implementation Units

Unit 1: Add first-class recipes for Nuxt, Astro, Remix, SvelteKit

Goal: Give the four "table stakes" JS frontend frameworks their own reference recipes with correct ports, start commands, and stub templates, so they stop falling through to unknown.

Requirements: R1, R6

Dependencies: None (recipe files are additive; they don't activate until Unit 2 extends the detector)

Files:

Create: plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-nuxt.md
Create: plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-astro.md
Create: plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-remix.md
Create: plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-sveltekit.md
Modify: plugins/compound-engineering/skills/ce-polish-beta/references/launch-json-schema.md (add 4 stub templates)

Approach:

Mirror the structure of dev-server-next.md exactly: Signature / Start command / Port / Stub generation / Common gotchas
Defaults per the current framework docs: Nuxt port 3000, Astro port 4321, Remix port 3000 (classic) or 5173 (Vite), SvelteKit port 5173
Each recipe's "Common gotchas" section notes interactions users will actually hit: Nuxt's Nitro, Astro's SSR vs SSG dev behavior, Remix's classic-vs-Vite fork, SvelteKit's adapter-free dev mode
Stub templates in launch-json-schema.md match the existing Next/Vite/Rails/Procfile pattern

Patterns to follow:

plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-next.md for overall shape
plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-vite.md for framework-on-Vite notes (relevant to SvelteKit and new Remix)

Test scenarios: Test expectation: none — reference markdown is consumed by the model, not asserted. Unit 5's integration test covers that these recipes are selected correctly when their respective signatures are present.

Verification:

Four new reference files exist with all five required sections
launch-json-schema.md has stub templates for all four new types
A reader landing on a new recipe can answer "what command do I run, at what port, with what launch.json stub?" without leaving the file
Unit 2: Extend detect-project-type.sh with new signatures and monorepo probe

Goal: The detector recognizes Nuxt/Astro/Remix/SvelteKit at the repo root and descends up to depth 3 into workspaces when root detection returns unknown, emitting <type> or <type>@<cwd> as appropriate.

Requirements: R1, R2, R5

Dependencies: Unit 1 (new types must have recipes before the detector returns them, so Phase 3.2 routing in Unit 5 doesn't dead-end)

Files:

Modify: plugins/compound-engineering/skills/ce-polish-beta/scripts/detect-project-type.sh
Create: tests/skills/ce-polish-beta-project-type.test.ts

Approach:

Keep the existing root-scan precedence block intact (rails beats procfile, single-match returns <type>)
Add signature checks for nuxt.config.{js,mjs,ts}, astro.config.{js,mjs,ts}, remix.config.{js,ts}, and svelte.config.{js,mjs,ts} at root
When the root-scan yields zero matches, run a shallow find with -maxdepth 3 excluding node_modules, .git, vendor, dist, build, coverage, .next, .nuxt, .svelte-kit, .turbo, tmp, fixtures looking for any supported signature filename
Collect hits as (type, relative-dir) pairs. Deduplicate on the pair
Single hit → emit <type>@<cwd> (or bare <type> when the hit is .)
Multiple hits → emit multiple:<type1>@<cwd1>,<type2>@<cwd2>,... (always include the type prefix so the grammar is unambiguous under naive awk -F: on the outer separator)
Zero monorepo hits → emit unknown unchanged
Header comment requirements: document the output grammar explicitly (the four concrete shapes: <type> / <type>@<cwd> / multiple / multiple:<type>@<cwd>,...), the depth cap of 3 with its rationale, and the exclusion list. Callers should not have to reverse-engineer the grammar from examples

Execution note: Test-first — add the new test file with scenarios for each new signature, monorepo single-hit, monorepo multi-hit, exclusion of node_modules, and the unchanged-root-detection regression cases. Run the suite red, then modify the detector to go green. This script is load-bearing for dev-server startup and has no production telemetry; tests are the only safety net.

Patterns to follow:

Existing detect-project-type.sh precedence block (rails-before-procfile)
tests/skills/ce-polish-beta-dev-server.test.ts for test harness shape

Test scenarios:

Happy path: nuxt.config.ts at root → nuxt
Happy path: astro.config.mjs at root → astro
Happy path: remix.config.js at root → remix
Happy path: svelte.config.js at root → sveltekit
Happy path: apps/web/next.config.js in Turborepo layout → next@apps/web
Happy path: packages/frontend/vite.config.ts in pnpm-workspace layout → vite@packages/frontend
Edge case: apps/web/next.config.js and apps/admin/next.config.js → multiple:next@apps/web,next@apps/admin
Edge case: apps/web/next.config.js and apps/api/Gemfile+bin/dev → multiple:next@apps/web,rails@apps/api
Edge case: signature inside node_modules/next/examples/... → ignored (root returns unknown)
Edge case: signature at depth 4 (projects/app/web/client/next.config.js) → ignored
Edge case: signature alongside bin/dev+Gemfile at root → returns rails (root wins, no probe runs)
Regression: existing 4-type root detection unchanged when signatures present at root
Regression: Procfile.dev + bin/dev + Gemfile → still returns rails, not multiple

Verification:

All 12 test scenarios pass
bash scripts/detect-project-type.sh run in a real Turborepo returns next@apps/web (or whichever app path matches)
Run in the plugin's own repo root still returns the existing detection (or unknown, matching prior behavior)
Unit 3: Package-manager resolver script

Goal: A new resolve-package-manager.sh emits the project's package manager (npm / pnpm / yarn / bun) plus the canonical dev-server argv, so the stub-writer can substitute both without in-agent judgment.

Requirements: R3, R6

Dependencies: None

Files:

Create: plugins/compound-engineering/skills/ce-polish-beta/scripts/resolve-package-manager.sh
Create: tests/skills/ce-polish-beta-package-manager.test.ts

Approach:

Accept an optional path as a positional argument (first positional); default to repo root via git rev-parse --show-toplevel when omitted
In the resolved path, check for lockfiles in priority order: pnpm-lock.yaml → yarn.lock → bun.lockb / bun.lock → package-lock.json
Emit two lines on stdout: line 1 = token (npm | pnpm | yarn | bun), line 2 = canonical command tail as a space-separated argv (e.g., run dev for npm/bun, dev for pnpm/yarn)
Fall through to npm + run dev only when a package.json is present and no lockfile matches (matches prior hardcoded behavior, so no regression for vanilla projects). If the path is a valid directory but contains no package.json, do not fall through to npm — emit the sentinel instead (see next bullet), so callers can distinguish "JavaScript project with no lockfile" from "not a JavaScript project at all"
If the path is a valid directory but contains no package.json, emit sentinel __NO_PACKAGE_JSON__ on stdout and exit 0 (expected-no-match, matching read-launch-json.sh sentinel convention — callers pattern-match on stdout, not exit code)
When both bun.lockb (binary) and bun.lock (text) are present in the same directory, prefer bun.lock (text). Rationale: Bun's text lockfile is the newer, canonical format; the binary format is a legacy variant. Only one will normally be present, but the resolver must deterministically pick one when both exist
If the path itself does not exist or is not a directory, emit ERROR: on stderr and exit 1 (operational failure, distinct from expected-no-match)
Header comment requirements: document the two-line stdout grammar (line 1 = binary, line 2 = argv tail), the lockfile priority order and why, and the sentinel-vs-error exit-code split

Patterns to follow:

plugins/compound-engineering/skills/ce-polish-beta/scripts/read-launch-json.sh for sentinel outputs and exit codes
Existing detect-project-type.sh for simple lockfile-presence checks

Test scenarios:

Happy path: pnpm-lock.yaml present → stdout: pnpm\ndev
Happy path: yarn.lock present → stdout: yarn\ndev
Happy path: bun.lockb present → stdout: bun\nrun dev
Happy path: bun.lock (text format) present → stdout: bun\nrun dev
Happy path: package-lock.json present → stdout: npm\nrun dev
Happy path: no lockfile, package.json present → stdout: npm\nrun dev (safe default)
Edge case: both pnpm-lock.yaml and yarn.lock present → stdout: pnpm\ndev (priority order wins)
Edge case: positional path pointing to apps/web — reads lockfile from subdir, not repo root
Edge case: positional path to a directory without package.json → stdout __NO_PACKAGE_JSON__, exit 0 (expected-no-match sentinel)
Edge case: no positional arg, not in a git repo → stderr ERROR: + exit 1 (operational failure)
Edge case: positional path but directory doesn't exist → stderr ERROR: + exit 1 (operational failure)

Verification:

All test scenarios pass
Running from a real pnpm repo returns pnpm\ndev
Running from a real npm repo returns npm\nrun dev
Unit 4: Port resolver script with authoritative config probes

Goal: A new resolve-port.sh probes config files in priority order (framework config → config/puma.rb → Procfile.dev → docker-compose.yml → package.json scripts → .env* → default), correctly parses .env values (stripping quotes and # comment), and drops the AGENTS.md/CLAUDE.md grep.

Requirements: R4, R6

Dependencies: None

Files:

Create: plugins/compound-engineering/skills/ce-polish-beta/scripts/resolve-port.sh
Create: tests/skills/ce-polish-beta-resolve-port.test.ts
Modify: plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-detection.md

Approach:

Accept optional positional path as the first positional argument (defaults to git rev-parse --show-toplevel when omitted) — consistent with parse-checklist.sh and the Unit 3 resolver
Accept optional --type <rails|next|vite|nuxt|astro|remix|sveltekit|procfile> flag to scope which probes run (e.g., skip config/puma.rb for Next). Type is a classification, not a path, so the flag form is appropriate and distinguishable from the positional path
Accept optional --port <n> flag as an explicit override (emit immediately when present, before any probing)
Probe order (first hit wins):
1. Explicit --port flag
2. Framework config: next.config.* / vite.config.* / nuxt.config.* / astro.config.* — conservative regex for port:\s*["']?[0-9]+["']? or server.port\s*=\s*[0-9]+. Numeric literals only; reject matches where the value is a variable reference (e.g., process.env.PORT, getPort()) so we do not emit a misleading default
3. Rails: config/puma.rb port\s+[0-9]+
4. Procfile: Procfile.dev web: line scanned for -p <n> / --port <n>
5. docker-compose.yml: in service named web / app / frontend, the first "<n>:<n>" line under ports:
6. package.json dev/start script for --port <n> / -p <n>
7. .env* files: check in override order .env.local → .env.development → .env (first hit wins, matching the convention most JS frameworks use where .env.local overrides .env.development which overrides .env). Parse PORT=<n>, stripping surrounding " or ' and truncating at # (after trimming whitespace)
8. Framework default (emitted from a lookup table: rails/next/nuxt/remix=3000, vite/sveltekit=5173, astro=4321, procfile=3000, unknown=3000)
Emit the resolved port as a single line on stdout. Do not emit provenance — stderr is reserved for ERROR: messages, matching the existing convention in read-launch-json.sh and parse-checklist.sh. If future debugging demand surfaces, add a --verbose mode in a follow-up rather than speculatively
Rewrite dev-server-detection.md: the inline bash cascade is removed; the file becomes a navigable pointer ("Port resolution runs via scripts/resolve-port.sh") plus the framework-default table and probe-order rationale. Include an explicit sync-note block listing the three intentional divergences from test-browser's inline cascade: (a) quote stripping on .env values, (b) comment stripping on .env values, (c) removal of the AGENTS.md/CLAUDE.md grep. The block tells a future maintainer of either skill exactly what not to "fix" back to symmetry
Header comment requirements: document the probe-order rationale (config-before-prose), the .env parsing contract (quote + comment stripping), and the reason AGENTS.md/CLAUDE.md grepping is deliberately omitted

Execution note: Test-first — .env parsing bugs are the whole point. Write cases for quoted, single-quoted, comment-trailed, whitespace-padded, and multi-line forms first. Implement against those cases.

Patterns to follow:

Existing cascade in references/dev-server-detection.md for probe order (improved, not replaced wholesale)
scripts/parse-checklist.sh for bash regex patterns and awk/sed composition
scripts/read-launch-json.sh for sentinel conventions and stderr-for-diagnostics

Test scenarios:

Happy path: --port 8080 explicit → 8080
Happy path: next.config.js with port: 4000 → 4000
Happy path: next.config.ts with server: { port: 4000 } → 4000
Happy path: config/puma.rb with port 3001 → 3001 (rails type)
Happy path: Procfile.dev web: bundle exec puma -p 4567 → 4567
Happy path: docker-compose.yml with web:\n ports:\n - "9000:9000" → 9000
Happy path: package.json "dev": "next dev --port 4000" → 4000
Edge case: .env PORT=3001 → 3001
Edge case: .env PORT="3001" → 3001 (quotes stripped)
Edge case: .env PORT='3001' → 3001 (single quotes stripped)
Edge case: .env PORT=3001 # dev only → 3001 (comment stripped)
Edge case: .env PORT="3001" # quoted+commented → 3001
Edge case: .env PORT = 3001 → 3001 (whitespace tolerated)
Edge case: .env.local PORT=4000 + .env PORT=3000 both present → 4000 (.env.local precedence)
Edge case: .env.development PORT=4000 + .env PORT=3000 both present → 4000 (.env.development precedence)
Edge case: .env.local PORT=4000 + .env.development PORT=5000 both present → 4000 (.env.local beats .env.development)
Edge case: multiple probes hit — framework config wins over .env (priority order)
Edge case: no probe matches, --type next → 3000 (default)
Edge case: no probe matches, --type vite → 5173
Edge case: no probe matches, --type astro → 4321
Edge case: no probe matches, no --type → 3000 (unknown default)
Error path: malformed docker-compose.yml — probe misses, falls through (no crash)
Error path: next.config.js with computed port (port: getPort()) — regex misses, falls through
Error path: next.config.js with port: process.env.PORT || 3000 — probe rejects the variable reference and falls through to .env / default (does not emit 3000 as if it were a framework-config hit)
Error path: positional path does not exist → stderr ERROR: + exit 1 (operational failure, not a fall-through)
Regression: AGENTS.md mentioning port 8443 in prose — ignored (grep removed)
Regression: CLAUDE.md mentioning localhost:3000 in examples — ignored

Verification:

All 20+ test scenarios pass
Running in the plugin's own repo root returns 3000 (default, since no framework config)
Running against a synthetic Rails repo with config/puma.rb port 3001 returns 3001
dev-server-detection.md no longer contains inline shell; it describes the probe order and framework-default table
Unit 5: Wire new scripts and signatures into SKILL.md Phase 3

Goal: SKILL.md Phase 3.2 routes the four new types and handles the <type>@<cwd> format; Phase 3.3 substitutes package-manager + cwd into stubs; port resolution calls resolve-port.sh instead of the inline cascade.

Requirements: R1, R2, R3, R4, R5

Dependencies: Units 1–4 (recipes, signatures, resolvers all exist)

Files:

Modify: plugins/compound-engineering/skills/ce-polish-beta/SKILL.md (Phase 3.2 routing table, Phase 3.3 stub-writer logic, references list at bottom)

Approach:

Phase 3.2 routing table gains four new rows (nuxt, astro, remix, sveltekit)
Phase 3.2 adds a paragraph under the table: "When the detector returns <type>@<cwd>, route by <type> as usual, and carry <cwd> into the stub-writer for Phase 3.3. When the detector returns multiple:<type1>@<cwd1>,<type2>@<cwd2>,..., the interactive prompt lists the <type>@<cwd> pairs and asks the user to pick one; headless mode emits the standard multiple failure with the pair list appended."
Phase 3.3 stub-writer logic updated: "For Next/Vite/Nuxt/Astro/Remix/SvelteKit stubs, call resolve-package-manager.sh (passing <cwd> as the positional arg when present) and substitute the emitted binary and args into runtimeExecutable / runtimeArgs. When the detector emitted <type>@<cwd>, populate the stub's cwd field with that value. For port, call resolve-port.sh [<cwd>] --type <type> and substitute the emitted port."
References list at the bottom of SKILL.md gains the three new reference files (Unit 1) and two new scripts (Units 3 and 4)
dev-server-detection.md reference in the "Cascade" section is kept but its description changes to "Port-resolution documentation — the runtime path is scripts/resolve-port.sh"

Patterns to follow:

Existing Phase 3.2 table structure and prose (keep the table format, add rows)
Existing Phase 3.3 stub-writer prose (keep imperative style, add substitution bullets)
Existing reference list at SKILL.md bottom (alphabetical within scripts/references groups)

Test scenarios:

Test expectation: none — SKILL.md content is model-consumed. The behavior it documents is asserted by Units 2, 3, and 4 unit tests.

Verification:

bun test tests/skills/ce-polish-beta-* passes (all old + new tests green)
bun run release:validate passes (SKILL.md structure intact, no broken references)
Reading SKILL.md Phase 3 start-to-finish, a reader can trace: "detector says next@apps/web" → "Phase 3.3 substitutes pm+port+cwd from resolvers into Next stub" → "final stub has cwd: apps/web, runtimeExecutable: pnpm, port: 3001"
Four new reference files and two new scripts appear in the SKILL.md references list

High-Level Technical Design

This illustrates the intended approach and is directional guidance for review, not implementation specification. The implementing agent should treat it as context, not code to reproduce.

Data flow through Phase 3 after the fix:

    .claude/launch.json exists? ──yes──▶ use it verbatim ──▶ Phase 3.5
          │
          no
          ▼
    detect-project-type.sh
          │
          ├─ rails | next | vite | procfile | nuxt | astro | remix | sveltekit
          │         │
          │         ▼
          │    load references/dev-server-<type>.md
          │    (recipe: command, default port, gotchas)
          │
          ├─ <type>@<cwd>     (monorepo hit, depth ≤ 3)
          │         │
          │         ▼
          │    load recipe + remember cwd for stub-writer
          │
          ├─ multiple[:<type>@<cwd>,...]   (disambiguation needed)
          │         │
          │         ▼
          │    interactive: user picks <type>@<cwd> pair
          │    headless: fail with pair list
          │
          └─ unknown          (no signature anywhere in scan scope)
                    │
                    ▼
               interactive: ask for exec/args/port
               headless: fail

    ── stub-writer (Phase 3.3) ──────────────────────────

    pm = resolve-package-manager.sh [<cwd>]   (Next/Vite/Nuxt/Astro/Remix/SvelteKit)
    port = resolve-port.sh [<cwd>] --type <type>

    stub = template(type).with(
             runtimeExecutable = pm.bin,
             runtimeArgs       = pm.args,
             port              = port,
             cwd               = cwd if present
           )

Probe-order for resolve-port.sh (first hit wins):

Rank	Source	Why this order
1	Explicit CLI `--port`	User intent is authoritative
2	Framework config (`next.config.` / `vite.config.` / `nuxt.config.` / `astro.config.`)	The framework itself reads this
3	`config/puma.rb` (rails only)	Rails server actually binds here
4	`Procfile.dev` web line	What `bin/dev` / foreman actually runs
5	`docker-compose.yml` web service ports	Container port binding, often authoritative in Docker-first dev
6	`package.json` `dev`/`start` scripts	Falls back to npm-style CLI flags
7	`.env*` (quote- and comment-stripped)	Env override, commonly used
8	Framework default	Last resort, documented table

System-Wide Impact

Interaction graph: Phase 3.2 routing consumes detector output; Phase 3.3 stub-writer consumes resolver output. No other phases touch these scripts. Headless mode's "never mutate state" invariant is preserved because all new scripts are read-only probes.
Error propagation: New scripts follow the sentinel-on-stdout + exit-code convention. Phase 3 already handles sentinel outputs from read-launch-json.sh; new sentinels (__NO_PACKAGE_JSON__) integrate into the same handler shape. Unknown probes fall through to framework defaults (same as today) rather than erroring.
State lifecycle risks: None. No persisted state changes; the stub-writer writes .claude/launch.json only in interactive mode with user consent (Phase 3.3 existing behavior, preserved).
API surface parity: Not applicable — this is a skill-internal detection subsystem. The skill's public contract (argument tokens, checklist.md format, headless envelope shape) is unchanged.
Integration coverage: Unit 5's verification explicitly traces a full monorepo + pnpm + custom-port scenario end-to-end to catch integration bugs the per-unit tests miss.
Unchanged invariants:
- .claude/launch.json always wins over auto-detect (Phase 3.1 unchanged)
- rails still beats procfile at root (existing precedence preserved)
- Headless mode still never writes .claude/launch.json
- The cross-skill dev-server-detection.md duplication note (vs test-browser) remains manual-sync; this plan does not modify test-browser

Risks & Dependencies

Risk	Mitigation
Monorepo probe false-positive (e.g., config in a fixture directory)	Exclusion list (`node_modules`, `fixtures`, etc.) in the probe; depth cap at 3; `multiple` output still triggers user disambiguation
Framework config regex misses a valid port (e.g., computed expression)	Falls through to `.env` then framework default — same as today, just with more chances to catch a literal. Documented as best-effort
Package-manager resolver picks wrong PM (e.g., stale `yarn.lock` in a pnpm-migrated repo)	Priority order follows common-case lockfile precedence; user can override via `launch.json`. Documented in the resolver's header comment
New test files slow the suite	Each new test file adds ~10-20 cases using the existing tmp-repo harness (already fast in `ce-polish-beta-dev-server.test.ts`); measurable impact expected < 2 seconds
Changing `dev-server-detection.md` breaks a downstream reader	The file is only referenced from within the skill; no external consumers. Grep confirms no cross-skill references before the change lands
Dropping `AGENTS.md`/`CLAUDE.md` port grep regresses users relying on it	Very low — the grep was added speculatively and the lossy pattern (`localhost:3000` match) makes it more likely to have surfaced wrong values than correct ones in the wild. Explicit `--port` and `.claude/launch.json` both remain as override paths
Polish's `resolve-port.sh` diverges from `test-browser`'s inline cascade and the two drift silently	Unit 4 adds an explicit sync-note block inside `dev-server-detection.md` enumerating the three intentional divergences (quote stripping, comment stripping, no `AGENTS.md`/`CLAUDE.md` grep). A future maintainer who "fixes" `test-browser` by copying polish's cascade, or vice versa, will hit the sync-note first. No automated cross-skill check — acceptable because both skills are internal and the cascade is small

Documentation / Operational Notes

Update PR description on #568 (or a follow-up PR) to note that these gaps are fixed and reference this plan
No marketplace release entry, version bump, or CHANGELOG edit — release-please handles it
No user-facing docs outside the skill's own reference tree
Keep dev-server-detection.md as a navigable doc explaining probe order + framework defaults, even though the implementation now lives in resolve-port.sh. Reviewers will still land there first when debugging port issues

Sources & References

Origin: PR feedback from @tmchow on EveryInc/compound-engineering-plugin#568 (comment)
Previous plan: docs/plans/2026-04-15-001-feat-ce-polish-skill-plan.md (feature this fixes)
Related files:
- plugins/compound-engineering/skills/ce-polish-beta/scripts/detect-project-type.sh
- plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-detection.md
- plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-next.md
- plugins/compound-engineering/skills/ce-polish-beta/references/dev-server-vite.md
- plugins/compound-engineering/skills/ce-polish-beta/references/launch-json-schema.md
- plugins/compound-engineering/skills/ce-polish-beta/SKILL.md (Phase 3)
Test harness pattern: tests/skills/ce-polish-beta-dev-server.test.ts

41 KiB Raw Blame History Unescape Escape

fix: Close ce-polish-beta detection gaps from PR #568 feedback

Overview

Problem Frame

Requirements Trace

Scope Boundaries

Context & Research

Relevant Code and Patterns

Institutional Learnings

Cross-Repo Reference (informational only)

Key Technical Decisions

Open Questions

Resolved During Planning

Deferred to Implementation

Implementation Units

High-Level Technical Design

System-Wide Impact

Risks & Dependencies

Documentation / Operational Notes

Sources & References

41 KiB

Raw Blame History