Commit Graph

325 Commits

  • fix: harden CI validators
    Ports personal-path validator hardening and quoted checkout detection onto current main.
  • fix: port hook session and dashboard safety fixes
    Ports suggest-compact session_id isolation and dashboard terminal/document launch safety onto current main.
  • fix: sync skill frontmatter and catalog counts
    Adds missing skill frontmatter, normalizes strict YAML metadata, syncs README catalog counts, and extends catalog validation for README/plugin/marketplace count drift.
  • fix(ci): flag SKILL.md frontmatter defects in validate-skills (#1669)
    * fix(ci): flag SKILL.md frontmatter defects in validate-skills
    
    Issue #1663 reported two SKILL.md frontmatter defects (missing `name:`
    on skill-stocktake; literal block-scalar `description: |-` on
    openclaw-persona-forge) that PR #1664 addresses at the data level.
    
    This change is complementary: it extends `scripts/ci/validate-skills.js`
    to catch the same class of defect statically going forward, so the
    frontmatter-vs-renderer problems do not silently reappear as new skills
    land.
    
    ## Checks added
    - Frontmatter must declare a `name:` field.
    - Frontmatter `description:` must not use a literal block scalar
      (`|` / `|-` / `|+`) — these preserve internal newlines and break
      flat-table renderers keyed off `description`. Folded (`>`) and inline
      strings are accepted.
    
    ## Behavior
    - Frontmatter findings default to WARN (exit 0) so this PR does not
      break CI while the two known offenders are still on main. Pass
      `--strict` or set `CI_STRICT_SKILLS=1` to promote them to ERROR
      (exit 1). Structural findings (missing / empty SKILL.md) remain
      errors as before.
    - Today against main, the validator reports exactly two warnings —
      the same two files called out in #1663 — and exits 0. When #1664
      lands, the validator reports zero warnings, at which point strict
      mode can be enabled in CI.
    
    ## Parser notes
    - Bespoke frontmatter parser mirrors the style of `validate-agents.js`
      (tolerant of UTF-8 BOM and CRLF; no new npm dependency).
    - Block-scalar continuation lines are skipped so keys inside a block
      scalar are not mistaken for top-level keys.
    - Hidden directories (`.something/`) under skills/ are now skipped.
    
    ## Tests
    Adds five focused tests to `tests/ci/validators.test.js`:
    - warns when frontmatter is missing `name` (default mode)
    - errors when frontmatter is missing `name` (--strict mode)
    - warns on literal block-scalar description (|-)
    - accepts folded (>) and inline descriptions under --strict
    - skips hidden directories under skills/
    
    ## Docs
    Adds two bullets to the `Skill Checklist` in CONTRIBUTING.md covering
    the two rules now surfaced by the validator.
    
    Refs #1663. Complements (does not compete with) #1664.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * fix(ci): harden SKILL.md frontmatter checks after bot review
    
    Address findings from CodeRabbit, Greptile, and cubic on #1669:
    
    - Guard empty or whitespace-only `name:` values. Previously
      `name:    ` silently passed because the presence check only
      tested key-set membership; now inspectFrontmatter captures
      trimmed values and validate flags an explicit 'name is empty'
      WARN/ERROR.
    - Broaden block-scalar detection to cover YAML 1.2 indent
      indicators (`|2`, `|-2`, `>2-`) and trailing comments
      (`|-  # note`). The old regex required a bare `|`/`>` with
      optional `+`/`-`, which let valid-but-disallowed forms slip
      through.
    - Update CONTRIBUTING.md checklist to list `|+` alongside `|`
      and `|-` for parity with the validator.
    - Extend runSkillsValidator to accept env overrides and add four
      regression tests: empty name, |+ description, |-2 + comment, and
      CI_STRICT_SKILLS=1.
    
    * fix(ci): address round-2 review on validate-skills frontmatter
    
    - Tighten extractFrontmatter closing delimiter to require a newline or
      end-of-file after the closing `---`, so body lines beginning with
      `---text` are not parsed as frontmatter (CodeRabbit).
    - Strip both trailing and comment-only values in inspectFrontmatter, so
      `name: # todo` is surfaced as empty rather than silently passing
      (cubic P2).
    - Extract validateSkillDir helper so the per-directory validation
      block moves out of validateSkills, keeping both functions under the
      50-line guideline (CodeRabbit nit).
    - Hoist runSkillsValidator to module scope in the test harness and
      share the spawnSync import with execFileSync so the helper stops
      re-requiring child_process on every invocation (CodeRabbit nit).
    - Add regression tests: comment-only `name:` values must fail strict
      mode; `---trailing` body lines must not be parsed as frontmatter.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * Update tests/ci/validators.test.js
    
    Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
  • fix(hooks): resolve MCP health-check spawn ENOENT on Windows (#1456)
    * fix(hooks): resolve MCP health-check spawn ENOENT on Windows
    
    On Windows, commands like 'npx' are batch files (npx.cmd) that require
    shell expansion to resolve via PATH. Without shell: true, Node.js
    spawn() fails with ENOENT.
    
    However, absolute paths (e.g. C:\Program Files\nodejs\node.exe) must
    NOT use shell mode because cmd.exe misparses paths containing spaces.
    
    Fix: enable shell mode only for non-absolute commands on Windows, using
    path.isAbsolute() to distinguish. This matches how attemptReconnect()
    already handles the shell option.
    
    Fixes #1455
    
    * fix(hooks): harden Windows shell spawn — validate command for metacharacters
    
    Addresses bot review feedback on PR #1456:
    
    - Add UNSAFE_SHELL_CHARS regex to guard against shell injection when
      needsShell=true: cmd.exe operators (&, |, <, >, ^, %, !, (), ;,
      whitespace) are rejected before shell mode is enabled
    - Add typeof command === 'string' check so path.isAbsolute() cannot
      throw on malformed non-string command values
    - Rename test to 'via PATH resolution' (not Windows-only; runs all platforms)
    - Fix misleading test comment: 'node' resolves via PATH like npx.cmd but
      does not itself use .cmd; comment now accurately reflects the intent
    
    * fix(hooks): kill full process tree on Windows when shell mode is used
    
    When needsShell=true, the spawned child is cmd.exe. Calling child.kill()
    only terminates the shell, leaving the real server process orphaned.
    
    Use taskkill /PID <pid> /T /F on Windows+shell to kill the entire
    process tree rooted at cmd.exe. Fall back to SIGTERM+SIGKILL on all
    other platforms or when shell mode is not active.
    
    * fix(hooks): fall back to child.kill() when taskkill fails
    
    Windows taskkill can fail if it's not on PATH, the process already
    exited, or permissions are denied. Previously the failure was silently
    ignored and no kill signal reached the child.
    
    Now: capture the spawnSync result and fall back to child.kill('SIGKILL')
    on any taskkill error or non-zero status. This still may leak a
    detached server process but at least guarantees the cmd.exe shell is
    signaled.
  • fix: install native Cursor hook and MCP config (#1543)
    * fix: install native cursor hook and MCP config
    
    * fix: avoid false healthy stdio mcp probes
  • Merge pull request #1495 from ratorin/fix/session-end-transcript-path-isolation
    fix(hooks): isolate session-end.js filename using transcript_path UUID (#1494)
  • fix(hooks): wrap SessionStart summary with stale-replay guard (#1536)
    The SessionStart hook injects the most recent *-session.tmp as
    additionalContext labelled only with 'Previous session summary:'.
    After a /compact boundary, the model frequently re-executes stale
    slash-skill invocations it finds inside that summary, re-running
    ARGUMENTS-bearing skills (e.g. /fw-task-new, /fw-raise-pr) with the
    last ARGUMENTS they saw.
    
    Observed on claude-opus-4-7 with ECC v1.9.0 on a firmware project:
    after compaction resume, the model spontaneously re-enters the prior
    skill with stale ARGUMENTS, duplicating GitHub issues, Notion tasks,
    and branches for work that is already merged.
    
    ECC cannot fix Claude Code's skill-state replay across compactions,
    but it can stop amplifying it. Wrap the injected summary in an
    explicit HISTORICAL REFERENCE ONLY preamble with a STALE-BY-DEFAULT
    contract and delimit the block with BEGIN/END markers so the model
    treats everything inside as frozen reference material.
    
    Tests: update the two hooks.test.js cases that asserted on the old
    'Previous session summary' literal to assert on the new guard
    preamble, the STALE-BY-DEFAULT contract, and both delimiters. 219/219
    tests pass locally.
    
    Tracked at: #1534
  • fix(gateguard): rewrite routineBashMsg to use fact-presentation pattern (#1531)
    * fix(gateguard): rewrite routineBashMsg to use fact-presentation pattern
    
    The imperative 'Quote user's instruction verbatim. Then retry.' phrasing
    triggers Claude Code's runtime anti-prompt-injection filter, deadlocking
    the first Bash call of every session. The sibling gates (edit, write,
    destructive) use multi-point fact-list framing that the runtime accepts.
    
    Align routineBashMsg with that pattern to restore the gate's intended
    behavior without changing run(), state schema, or any public API.
    
    Closes #1530
    
    * docs(gateguard): sync SKILL.md routine gate spec with new message format
    
    CodeRabbit flagged that skills/gateguard/SKILL.md still described the
    pre-fix imperative message. Update the Routine Bash Gate section to
    match the numbered fact-list format used by the new routineBashMsg().
  • fix(scripts): resolve claude.cmd on Windows by enabling shell for spawn (#1471)
    Fixes #1469.
    
    On Windows the `claude` binary installed via `npm i -g @anthropic-ai/claude-code`
    is `claude.cmd`, and Node's spawn() cannot resolve .cmd wrappers via PATH
    without shell: true. The call failed with `spawn claude ENOENT` and claw.js
    returned an error string to the caller.
    
    Mirrors the fix pattern applied in PR #1456 for the MCP health-check hook.
    'claude' is a hardcoded literal (not user input), so enabling shell on Windows
    only is safe.
  • review: broaden CLAUDE_TRANSCRIPT_PATH fallback to cover missing/empty JSON fields
    Previously the env fallback ran only when JSON.parse threw. If stdin was valid
    JSON but omitted transcript_path or provided a non-string/empty value, the
    script dropped to the getSessionIdShort() fallback path, re-introducing the
    collision this PR targets.
    
    Validate the parsed transcript_path and apply the env-var fallback for any
    unusable value, not just malformed JSON. Matches coderabbit's outside-diff
    suggestion and keeps both input-source paths equivalent.
    
    Refs #1494
  • review: apply sanitizeSessionId to UUID shortId, fix test comment
    - Route the transcript-derived shortId through sanitizeSessionId so the
      fallback and transcript branches remain byte-for-byte equivalent for any
      non-UUID session IDs that still land in CLAUDE_SESSION_ID (greptile P1).
    - Clarify the inline comment in the first regression test: clearing
      CLAUDE_SESSION_ID exercises the transcript_path branch, not the
      getSessionIdShort() fallback (coderabbit P2).
    
    Refs #1494
  • review: address P1/P2 bot feedback on shortId derivation
    - Use last-8 chars of transcript UUID instead of first-8, matching
      getSessionIdShort()'s .slice(-8) convention. Same session now produces the
      same filename whether shortId comes from CLAUDE_SESSION_ID or transcript_path,
      so existing .tmp files are not orphaned on upgrade.
    - Normalize extracted hex prefix to lowercase to avoid case-driven filename
      divergence from sanitizeSessionId()'s lowercase output.
    - Explicitly clear CLAUDE_SESSION_ID in the first regression test so the env
      leak from parent test runs cannot hide the fallback path.
    - Add regression tests for the lowercase-normalization path and for the case
      where CLAUDE_SESSION_ID and transcript_path refer to the same UUID (backward
      compat guarantee).
    
    Refs #1494