chore(ship): default to chatgpt planner, fix #630 tooling friction - #654
Merged
Conversation
Learned across #630's 8 phases: - --planner now defaults to chatgpt (ChatGPT authors/revises, Fable/high approves); --planner fable opts back into the original mode. Flipped every default/opt-in reference in SKILL.md, per-issue-cycle.md, and review-loops.md accordingly. - code-review-pass.workflow.mjs's own up-to-4-attempt retry logic makes real chatgpt-review CLI calls that count against the tool's hard 3-total-passes-per- session cap, invisibly to the coordinator's own pass count -- observed on PR #653 where pass 1 silently spent 2 of 3 slots. Added attemptsUsed + a dedicated sessionExhausted/session-cap-exhausted status so this is detected immediately instead of discovered as a bare CLI rejection. - Documented a recurring, previously undocumented ChatGPT-side failure (a live tool call hanging mid-turn, distinct from the completion-detection bug fixed in #650) and its recovery (stop + nudge in the same conversation) as an explicit review-loops.md procedure to try before treating needs_human as SKILL.md's FULL STOP. - SKILL.md step 2.7: gh pr merge --delete-branch's local main fast-forward can fail even when the remote merge succeeds, since a /ship run never advances local main itself (every unit branches off origin/main directly) -- added an explicit check+repair step. - per-issue-cycle.md reviewer prompt + repo-footguns.md: the escape-unaware-prefilter and node_modules-symlink-bypass bug classes recurred repeatedly against build/check-boundaries.mjs across #630 -- added explicit checklist coverage. - chatgpt-plan-author-loop.workflow.mjs: capture lastResponsePreview on an incomplete authoring result, for diagnosing a future recurrence of #630 phase 8's one never-root-caused needs_human. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz
3 tasks
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
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
Reviewed the
/shipskill against the full 8-phase history of issue #630 (the@altinity/clickhouse-httpextraction) and made the fixes that history pointed at, plus flipped the default planner per explicit request.chatgpt(--planner fableopts back into the original Fable-authors/ChatGPT-reviews mode). Updated every default/opt-in reference acrossSKILL.md,per-issue-cycle.md, andreview-loops.md.code-review-pass.workflow.mjs): the review-runner's own internal retry logic (up to 4 attempts per/ship-labelled pass) makes realchatgpt-review prCLI calls that count against the tool's hard 3-total-passes-per-session cap — invisibly to the coordinator's own pass count. Observed live on PR feat(#630): make @altinity/clickhouse-http standalone-buildable, retire client-web vendor spike (phase 8) #653: "pass 1" silently spent 2 of 3 slots on an internal retry, so "pass 3" was rejected outright as CLI call 4. AddedattemptsUsed+ a dedicatedsessionExhausted/session-cap-exhaustedstatus so this is detected immediately rather than discovered as a bare rejection.review-loops.md): a live tool call hanging mid-turn for 5-10+ minutes hit every plan-authoring session from Phase 3 onward — distinct from the completion-detection bug already fixed in fix(chatgpt-review): plan-author early-exit once a complete protocol appears #650. Previously handled ad hoc each time; now an explicit procedure (stop + nudge in the same conversation) to try before treatingneeds_humanasSKILL.md's FULL STOP.maindrift check+repair (SKILL.md2.7): a/shiprun never advances localmain(every unit branches offorigin/maindirectly), sogh pr merge --delete-branch's local fast-forward step can fail even when the remote merge succeeds — happened on PR feat(#630): make @altinity/clickhouse-http standalone-buildable, retire client-web vendor spike (phase 8) #653, the run's very last merge. Added an explicit check+repair step.node_modules-workspace-symlink-bypass bug classes each recurred multiple times againstbuild/check-boundaries.mjsacross Epic: extract the Fetch-native ClickHouse client into a reusable package #630 (twice on the same guard within Phase 8 alone) — added explicit checklist coverage so a reviewer checks every sibling instance, not just the one flagged.chatgpt-plan-author-loop.workflow.mjs): Phase 8 hit oneneeds_humanthat never got root-caused (a complete plan seemingly not recognized). Now captures the last response text on an incomplete result so a recurrence is easier to diagnose.Known pre-existing issue (not touched here)
skills/ship/tests/workflow-contract.test.mjshas one pre-existing failing assertion (a stale drift-bind regex againstdecompose-and-implement-loop.workflow.mjsthat no longer matches its content since PR #651's wave-based rewrite) — confirmed viagit stashthat it fails identically onorigin/main, unrelated to this change. Left alone as a separate concern.Test plan
node --test skills/ship/tests/*.test.mjs— 7/8 pass; the 1 failure is the pre-existing, unrelated issue above.workflow.mjsfiles syntax-checked (wrapped to match the Workflow runtime's actual execution shape, since they use top-levelreturn/agent/log)invocation.test.mjsupdated and passing for the new default + explicit opt-outtests//build/trees into.claude/skillsorskills/ship— this change cannot affect the main gate🤖 Generated with Claude Code
https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz