chore(build): parser kind field fix, bump build skill v1.22.0#1445
Open
anbangr wants to merge 209 commits into
Conversation
# Conflicts: # README.md
- Adds implement/SKILL.md.tmpl to execute plans in phases - Updates GSTACK_PLAYBOOK.md to include the new workflow
…s review and ship, add implement reexamine mode
… and sonnet for review/qa
…erative fix, and deployment
… review instead of /review
- Update Step M3 monitor launch to use set -o pipefail and
${PIPESTATUS[0]} while teeing output to monitor-output.log
- Add Step M3.5 that scans monitor output for SKILL_FAULT_DETECTED,
dedupes by resolved path (readlink), reads fault_investigator_model
from configure.cm, and dispatches either GSTACK_FAULT_INVESTIGATOR_COMMAND
or one background agent per non-duplicate fault
- Add validation tests for Step M3.5 content in skill-md.test.ts
- Fix pre-existing hardcoded model name in cli.ts comment
…_RUN_ID The previous Step M3.5 implementation had a critical silent-failure bug: 1. `sed -n 's/.*file:////p'` is a malformed sed expression (4 slashes = bad flag in substitute command). `_FAULT_FILE` was always empty and the `[ -z "$_FAULT_FILE" ] && continue` guard silently skipped every fault. 2. The expression also assumed a `file://` URI format that the monitor never emits — actual SKILL_FAULT_DETECTED events are JSON lines with a `faults[].sourceFiles[]` array (see build/orchestrator/cli.ts:5739-5741 and build/orchestrator/types.ts:15-23). No investigator would ever spawn. Switch to jq-based JSON parsing that flattens each event into TSV rows (runId<TAB>category<TAB>file) and pass FAULT_CATEGORY + FAULT_RUN_ID env vars to the investigator alongside FAULT_FILE. Dedupe key now includes (runId, category, resolved-path) so unrelated faults across runs aren't collapsed. Log filename is suffixed with category to avoid collisions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…atch Adds test/skill-e2e-build-fault-investigator.test.ts (periodic tier) covering the fault investigator E2E flow: mock gstack-build outputs SKILL_FAULT_DETECTED JSON, Step M3.5 dispatches GSTACK_FAULT_INVESTIGATOR_COMMAND with fault env vars, mock investigator writes report to $FAULT_PRIMARY, assertions verify report exists with PLAN_SYNTHESIS_INVALID and no source files were edited. Registers build-fault-investigator-e2e in touchfiles.ts — selected when build/SKILL.md, skill-fault-detector.ts, or monitor.ts change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the `--skip-sweep` flag and the unshipped feat/* sweep bullet from the Startup Gates section and flags table. Aligns with the code removal in 3e2b8b2. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Adds mock configure.cm file to prevent jq from failing in Step M3.5 mock
…11-074503-fabe4c3f-4-e2e-test-touchfile-registration'
1. plan-selection (6 tests): `defaultActiveRunRegistryDir()` hardcoded `~/.gstack/build-state/active-runs` and ignored `GSTACK_BUILD_STATE_DIR`, causing 11 real active-run records to leak into unit tests and inflate candidate counts (turning expected "selected" into "ambiguous"). Fix: honour the env var consistently, the same way `state.ts` already does. 2. integration (3 tests): plan review subprocess called `codex` with `OPENAI_API_KEY` from the inherited `process.env`, triggering a real ~30s API call against the LLM. These tests exercise feature lifecycle, not plan review. Fix: add `--no-plan-review` to each CLI invocation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…estSpec detection Four improvements identified during code review of 3e2b8b2: - Move `extractCoverageTarget` from cli.ts to sub-agents.ts (alongside parseCoveragePercent); re-export via import in cli.ts. Eliminates the circular-import risk when phase-runner.ts calls coverage functions. - Fix decimal truncation in extractCoverageTarget: `(\d+)` only matched integers, silently returning 80 for targets like ≥90.5%. Changed to `([\d.]+)` + parseFloat. - Fix `hasTestSpec` detection in buildGeminiTestSpecPrompt: was `phase.body.includes("#### Test Spec")` (fragile string match, false negative when body text differs). Now `phase.testSpecCheckboxLine !== -1` (parser already computes this — zero extra overhead). - Wire coverage gate in RUN_TESTS handler: after GREEN tests pass and the phase has a test spec (`testSpecCheckboxLine !== -1`), call parseCoveragePercent(result.stdout, testCmd) and compare against extractCoverageTarget(phase.body). Below target → set coverageResult and route to test_fix_running. Unknown framework → log advisory, proceed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Complete the coverage gate: `injectCoverageFlags(testCmd)` appends the appropriate flag for the detected framework before the GREEN test run, so `parseCoveragePercent` reliably finds coverage data in stdout even when projects don't pre-configure coverage in their test script. Framework → flag mapping: jest → --coverage --coverageReporters text vitest → --coverage bun test → --coverage pytest → --cov --cov-report term-missing go test → -cover unknown → unchanged (advisory log, gate skips) Injection is idempotent (no-op if flag already present) and only fires when the phase has a test spec (testSpecCheckboxLine !== -1) — VERIFY_RED and legacy phases use the bare test command unchanged. 11 unit tests added covering each framework, idempotency, and unknowns. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
`phase.kind !== "code" ? "" : ""` always evaluated to "" regardless of the condition, and was silently filtered by .filter(Boolean). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d, document sweep removal
…p (Bug D1) Two failing tests document the bug: 1. After CRITICAL verdict, state.planReview must be persisted with status "critical_exit_pending" — currently cli.ts does not persist anything before process.exit(3), so planReview stays undefined on disk. 2. On resume with the sentinel set, the plan-review gate must still fire — the current guard (!state.planReview) is false when planReview is truthy, so the gate is skipped after the sentinel is introduced. Two GREEN tests confirm baseline behavior: APPROVE verdict suppresses the gate; undefined planReview (first run) fires the gate. Tests MUST fail until Feature 4 implementation lands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Before this fix, a CRITICAL plan-review verdict caused process.exit(3)
without saving any sentinel to state. On resume, !state.planReview was
true → review ran again → CRITICAL again → infinite loop.
Fix:
1. Save state.planReview = { ...verdict, status: "critical_exit_pending" }
before releaseLock + process.exit(3) so the sentinel survives on disk.
2. Widen the plan-review gate guard from !state.planReview to
!state.planReview || state.planReview.status === "critical_exit_pending"
so the gate re-fires on resume when the sentinel is present.
Tests: two new tests in phase-runner.test.ts cover both the sentinel
persistence and the widened gate; 90/90 passing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g D2) Introduces ExitError (errors.ts) — thrown instead of process.exit(N) inside try/finally blocks so the finally clause runs cleanup before the process terminates. Changes: - errors.ts: new ExitError class (instanceof Error, numeric code field) - cli.ts: import ExitError; replace critical_exit process.exit(3) with throw new ExitError(3); update main().catch to call process.exit(err.code) when err instanceof ExitError - phase-runner.test.ts: 5 new tests (ExitError shape, propagation through finally, default and custom messages); 95/95 passing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ature 6) applyResult() now populates phaseState.coverageResult when: - action is RUN_TESTS - tests are GREEN (status = "tests_green") - extra.phaseBody is provided - parseCoveragePercent() returns a non-null value for the stdout Coverage below target emits an advisory warning but keeps status "tests_green" — not blocking. The target defaults to 80 when no "**Coverage target: ≥N%**" line appears in the phase body. 6 new tests in phase-runner.test.ts; 101/101 passing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ics + test assertions - Add errors.ts to MODULE_TEST_OWNERS in coverage-matrix.test.ts - Fix analytics logActivity to emit "success" for exit code 13 (FINALIZATION_REQUIRED), which is a success state (pending ship), not a failure - Fix integration test assertions: --skip-ship correctly exits 13, not 0, when features reach origin_verified (pre-existing test/impl mismatch)
…d [Phase 1.1] RED phase TDD: 11 tests fail because the parser does not yet stamp kind: "code" on emitted phases, and existing Phase literal construction sites have no kind field (undefined fails the VALID_KINDS.includes runtime assertion). 11 tests pass immediately: direct Phase construction with explicit kind values, and PhaseKind union membership checks (both already exist in types.ts). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add required kind: PhaseKind field to the parser factory init and to every Phase literal construction site in tests/fixtures. This ensures backward-compatible default of kind: "code" for all existing phases while the type system enforces correctness going forward. - parser.ts: stamp kind: "code" on every emitted Phase - state.test.ts, cli.test.ts, phase-runner.test.ts, feature-review.test.ts, cli-guardrails.test.ts, phase-kind.test.ts: add kind: "code" to all helpers and inline literals
…tations - Fix PHASE_HEADING regex to allow optional [kind] bracket between number and colon - Add BODY_KIND_PATTERN for <!-- kind: X --> HTML comment fallback - Add IMPL_LABELS_BY_KIND and REVIEW_LABELS_BY_KIND maps for all 5 PhaseKind values - Parser now stamps kind from heading bracket (primary), body comment (fallback), or defaults to "code" - Inline kind-comment detection ensures kind is set before checkbox processing - Add implCheckboxRe/reviewCheckboxRe for kind-specific checkbox matching - Add 16 new parser tests covering all bracket annotations, HTML fallback, checkbox recognition Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add IMPL_MARKER_BY_KIND and REVIEW_MARKER_BY_KIND lookup tables - Update flipPhaseCheckboxes signature to accept optional kind?: PhaseKind - Derives implMarker/reviewMarker from kind ?? "code" (backward compat) - Update reconcilePhaseCheckboxes to pass phase.kind - Update both cli.ts call sites (lines ~3870, ~4282) to pass kind: phase.kind - Add 9 kind-aware mutator tests covering all 5 kinds + error cases + backward compat Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…EW gates, ship gate
…(p as any) cast - Merge origin/main (13 commits behind, real conflict in parser.test.ts) - Resolve parser.test.ts conflict in favor of main's comprehensive PhaseKind test matrix (writing, experiment, research, manual, malformed bracket warning, HTML comment fallback, checkbox line tests) - Drop branch's (p as any).kind cast in parser.ts; use typed p.kind ?? code - Regenerate SKILL.md docs for all hosts after merge
…-storm-20260511-122548-fabe4c3f-3-commit-housekeeping-parser-fix-readme-regenerated-ski # Conflicts: # test/gen-skill-docs.test.ts # test/helpers/touchfiles.ts
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
This PR contains housekeeping changes for the build skill:
Pre-Landing Review
No issues found.
Test Coverage
Tests: All existing tests pass (1188 pass, 0 fail).
Need help on this PR? Tag
@codesmithwith what you need.