feat: Comet Auto-Pilot — SessionStart Hook + autonomous pipeline#54
feat: Comet Auto-Pilot — SessionStart Hook + autonomous pipeline#54chentao326 wants to merge 7 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Comet Auto-Pilot (v0.4.0), which automates the five-phase development pipeline. It adds a new comet-auto skill, integrates a SessionStart hook across multiple platforms to auto-resume incomplete changes, and provides a CLI command to manage configurations. The review feedback highlights several critical issues: an undefined cross_phase_failures variable in the English build skill, a directory-to-file type conflict for .comet-auto-active that breaks cycle guards, Windows path escaping bugs in run-hook.cmd, an incorrect regex pattern for retry decisions in the Chinese build skill, and configuration validation regexes that mistakenly match commented-out lines.
| failure_count=0 | ||
| max_retry=${auto_config.max_retry:-2} | ||
| max_consecutive=${auto_config.max_consecutive_failures:-5} | ||
|
|
There was a problem hiding this comment.
The variable cross_phase_failures is used on line 103 to check consecutive failures, but it is never defined or initialized in this English version of the skill file (unlike the Chinese version). This will cause execution to fail or evaluate incorrectly. Define it by counting the retries from the decisions log.
| failure_count=0 | |
| max_retry=${auto_config.max_retry:-2} | |
| max_consecutive=${auto_config.max_consecutive_failures:-5} | |
| failure_count=0 | |
| max_retry=${auto_config.max_retry:-2} | |
| max_consecutive=${auto_config.max_consecutive_failures:-5} | |
| cross_phase_failures=$(grep -c '"decision":"retry_' openspec/changes/<name>/.comet/auto/decisions.jsonl 2>/dev/null || echo "0") | |
There was a problem hiding this comment.
已修复于 c88bd47：在 failure_count/max_retry 定义后添加了 cross_phase_failures= 定义行。
| mkdir -p openspec/changes/.comet-auto-active | ||
| echo "<current-change-name>" > openspec/changes/.comet-auto-active/.active |
There was a problem hiding this comment.
Creating .comet-auto-active as a directory (mkdir -p) and writing to .comet-auto-active/.active conflicts with other parts of the codebase (such as hooks/session-start, comet-archive/SKILL.md, and src/commands/auto.ts) which treat .comet-auto-active as a regular file (using [ -f ], rm -f, and fs.unlink). This mismatch breaks the cycle guard and causes EISDIR errors during cleanup. Write directly to the file instead.
| mkdir -p openspec/changes/.comet-auto-active | |
| echo "<current-change-name>" > openspec/changes/.comet-auto-active/.active | |
| echo "<current-change-name>" > openspec/changes/.comet-auto-active |
There was a problem hiding this comment.
已修复于 c88bd47：将 mkdir -p openspec/changes/.comet-auto-active + echo > .active 改为直接 echo "" > openspec/changes/.comet-auto-active，统一作为普通文件使用。
| mkdir -p openspec/changes/.comet-auto-active | ||
| echo "<current-change-name>" > openspec/changes/.comet-auto-active/.active |
There was a problem hiding this comment.
Creating .comet-auto-active as a directory (mkdir -p) and writing to .comet-auto-active/.active conflicts with other parts of the codebase (such as hooks/session-start, comet-archive/SKILL.md, and src/commands/auto.ts) which treat .comet-auto-active as a regular file (using [ -f ], rm -f, and fs.unlink). This mismatch breaks the cycle guard and causes EISDIR errors during cleanup. Write directly to the file instead.
| mkdir -p openspec/changes/.comet-auto-active | |
| echo "<current-change-name>" > openspec/changes/.comet-auto-active/.active | |
| echo "<current-change-name>" > openspec/changes/.comet-auto-active |
| set SCRIPT_DIR=%~dp0 | ||
|
|
There was a problem hiding this comment.
On Windows, %SCRIPT_DIR% contains backslashes (e.g., C:\path\to\hooks\). Passing this directly to bash causes backslashes to be treated as escape characters, breaking execution. Replacing backslashes with forward slashes ensures compatibility.
set SCRIPT_DIR=%~dp0
set SCRIPT_DIR=%SCRIPT_DIR:\=/%
There was a problem hiding this comment.
已修复于 c88bd47：在 set SCRIPT_DIR=%~dp0 后添加 set SCRIPT_DIR=%SCRIPT_DIR:=/% 转换反斜杠。
| failure_count=0 | ||
| max_retry=${auto_config.max_retry:-2} | ||
| consecutive_failures=${auto_config.max_consecutive_failures:-5} | ||
| cross_phase_failures=$(grep -c '"decision":"retry_*"' openspec/changes/<name>/.comet/auto/decisions.jsonl 2>/dev/null || echo "0") |
There was a problem hiding this comment.
The regex pattern "decision":"retry_*" uses basic regular expression (BRE) syntax where * matches zero or more of the preceding character (the underscore _), rather than matching any character sequence. This will fail to match specific decisions like retry_build_error. Use "decision":"retry_" instead to correctly match any retry decision, aligning with the pattern used in comet-verify/SKILL.md.
| cross_phase_failures=$(grep -c '"decision":"retry_*"' openspec/changes/<name>/.comet/auto/decisions.jsonl 2>/dev/null || echo "0") | |
| cross_phase_failures=$(grep -c '"decision":"retry_"' openspec/changes/<name>/.comet/auto/decisions.jsonl 2>/dev/null || echo "0") |
There was a problem hiding this comment.
已修复于 c88bd47：将 grep -c '"decision":"retry_*"' 修正为 grep -c '"decision":"retry_"'，与 comet-verify 保持一致。
| } | ||
|
|
||
| // Validate confirm_design values | ||
| const confirmMatch = content.match(/confirm_design:\s*(\S+)/); |
There was a problem hiding this comment.
The regex /confirm_design:\s*(\S+)/ lacks the multiline flag (m) and the start-of-line anchor (^). This causes it to match commented-out lines (e.g., # confirm_design: auto_with_diff in the description/comments), leading to false positives during validation. Use a multiline regex with a start-of-line anchor to ensure only active configurations are matched.
| const confirmMatch = content.match(/confirm_design:\s*(\S+)/); | |
| const confirmMatch = content.match(/^\s*confirm_design:\s*([^\s#]+)/m); |
There was a problem hiding this comment.
已修复于 c88bd47：confirm_design 正则改为 /^\sconfirm_design:\s([^\s#]+)/m。
| } | ||
|
|
||
| // Validate isolation values | ||
| const isolationMatch = content.match(/isolation:\s*(\S+)/); |
There was a problem hiding this comment.
The regex /isolation:\s*(\S+)/ lacks the multiline flag (m) and the start-of-line anchor (^). This causes it to match commented-out lines (e.g., # isolation: branch in the description/comments), leading to false positives during validation. Use a multiline regex with a start-of-line anchor to ensure only active configurations are matched.
| const isolationMatch = content.match(/isolation:\s*(\S+)/); | |
| const isolationMatch = content.match(/^\s*isolation:\s*([^\s#]+)/m); |
There was a problem hiding this comment.
已修复于 c88bd47：isolation 正则改为 /^\sisolation:\s([^\s#]+)/m。
- fix(comet-build EN): add missing cross_phase_failures definition - fix(comet-auto EN+ZH): change .comet-auto-active from directory to file, fixing EISDIR errors in cycle guard cleanup - fix(run-hook.cmd): escape Windows backslashes for bash compatibility - fix(comet-build ZH): correct grep regex from retry_* to retry_ - fix(auto.ts): add ^ anchor and /m flag to confirm_design/isolation regexes to avoid matching commented lines
- Add SessionStart hook system (6 platform support) - Add comet-auto skill for autonomous pipeline execution - Add three-state design confirmation (auto_with_diff/always_confirm/always_skip) - Add retry strategy with per-phase and cross-phase failure limits - Add cycle prevention via .comet/auto/.active file marker - Add audit logging (JSONL), dry-run mode, design rollback - Add comet auto CLI command for configuration - Add auto field to .comet.yaml KNOWN_KEYS - Bump version to 0.4.0
- Add Auto-Pilot mode section to English comet skill - Add three-state design confirmation to English comet-design - Add auto decision skipping and retry logic to English comet-build - Add cross-phase failure counting to English comet-verify - Add auto archive to English comet-archive - Replace stub with full English comet-auto skill
- fix(comet-build EN): add missing cross_phase_failures definition - fix(comet-auto EN+ZH): change .comet-auto-active from directory to file, fixing EISDIR errors in cycle guard cleanup - fix(run-hook.cmd): escape Windows backslashes for bash compatibility - fix(comet-build ZH): correct grep regex from retry_* to retry_ - fix(auto.ts): add ^ anchor and /m flag to confirm_design/isolation regexes to avoid matching commented lines
chentao326
force-pushed
the
codex/comet-auto-pilot
branch
from
c88bd47 to
6a15261
Compare
- Run prettier on src/commands/auto.ts to fix CI format check - Add 'AskUserQuestion tool' to design confirmation enforcement text in both ZH and EN comet-design skill files to match PR rpamis#51 test expectations
benym
left a comment
benym
left a comment
There was a problem hiding this comment.
Code Review — Request Changes
Thanks for this well-designed feature. The Auto-Pilot concept, three-state design confirmation, cycle prevention, and audit logging are all thoughtfully architected.
However, I found 5 HIGH-severity issues that need fixing before merge:
HIGH Issues (must fix)
H1: Stale cross_phase_failures counter in retry loops
assets/skills/comet-build/SKILL.md, assets/skills-zh/comet-build/SKILL.md
cross_phase_failures is computed once before the while loop but each retry appends to decisions.jsonl. After the first iteration, the in-memory variable is stale — the cross-phase failure limit becomes ineffective.
→ Fix: Re-compute cross_phase_failures inside the loop body.
H2: Config regex matches commented-out lines
hooks/session-start line ~29: grep -E '^\s*enabled:' matches # enabled: false commented lines. A user commenting out the field would still have it parsed.
→ Fix: grep -E '^[^#]*enabled:'
H3: Same regex issue in auto.ts validateConfig
src/commands/auto.ts: content.match(/^\s*confirm_design:/m) matches # confirm_design: lines.
→ Fix: Use /^[^#]*confirm_design:/m
H4: sha256sum not cross-platform
comet-design/SKILL.md (EN + ZH) uses sha256sum directly. Per CLAUDE.md rules, must support both sha256sum (GNU) and shasum -a 256 (BSD/macOS).
→ Fix: Add the standard cross-platform hash pattern.
H5: Undocumented python3 hard dependency
hooks/session-start depends on python3 for JSON parsing. If not installed, all fallbacks silently report 0 changes, causing the hook to exit without context injection. This dependency is not documented.
→ Fix: Document as requirement, or add pure-shell fallback.
MEDIUM Issues (recommended)
- M1: Hardcoded Chinese in
session-starthook andauto.tsCLI — should default to English for a 6-platform tool - M2: Duplicate
max_consecutive_failures: 5— appears both at top-level and insidethresholds: - M3:
hooks.jsonduplicateshooks-claude.json(identical content) - M4: Unused
import os from 'os'inauto.ts - M5:
manifest.jsonversion was behind (0.3.2 vs 0.3.5) before this PR — pre-existing - M6: Auto-Pilot auto-selects "keep branch" during verify — significant behavior not prominently disclosed
- M7:
escape_for_jsondoesn't handle all control characters
LOW Issues (optional)
- L1: No test coverage for new
auto.tscommand (159 existing pass, 0 new added) - L2:
run-hook.cmdonly handlessession-start, not extensible - L3: Minor markdown formatting inconsistencies in modified skills
Full review artifact: .claude/reviews/pr-54-review.md
|
I generated the review using Claude. Please check it first for these issues. |
…ting - Restore 6 unchanged skill files (hotfix/tweak/open ZH+EN) from upstream - Add AskUserQuestion enforcement text to comet-design/verify/build (ZH+EN) - Fix EN design case (Must -> must) for test assertion matching - Add create independent change through /comet-open to EN build - Add skill tool loading references for worktree/brainstorming in EN build - Fix prettier formatting for src/commands/auto.ts
HIGH fixes: - H1: Recompute cross_phase_failures inside retry loop (EN+ZH) - H2: Fix grep regex to exclude commented lines in session-start - H3: Fix confirm_design/isolation regex to exclude commented lines - H4: Add cross-platform sha256sum fallback in ZH comet-design - H5: Document python3 dependency in README MEDIUM fixes: - M1: Translate all Chinese strings to English (auto.ts + session-start) - M2: Remove duplicate max_consecutive_failures from thresholds - M3: Remove redundant hooks.json (6 platforms have own files) - M4: Remove unused import os from auto.ts - M6: Add prominent Auto-Pilot keep-branch disclosure in verify skills - M7: Handle more control chars in escape_for_json LOW fixes: - L2: Make run-hook.cmd extensible via auto-discovery - L3: Clean trailing whitespace in modified SKILL.md files
Review fixes applied ✅@benym Thanks for the thorough review! All issues have been addressed in HIGH (all fixed)
MEDIUM (all fixed)
LOW (all fixed)
Ready for re-review! |
|
I think the idea behind this PR is good, but there are too many changes to the main process, losing many details from the previous version.
Due to the numerous issues introduced in this version (even too many to count), there is a lack of consideration for multiple rounds of conversations regarding the core mechanism. I suggest adopting the minimum modification mode to support this scenario, or starting with simple PRs in the community to familiarize with the project. Large-scale changes are not suitable for a one-time push. |
2 participants
Summary
Adds Auto-Pilot mode to Comet, enabling automatic change resumption and fully autonomous pipeline execution.
What it does
comet-autoSkill: Core auto-pilot logic — discovers changes, sorts by priority, routes to appropriate phase skillsauto_with_diff(audit trail),always_confirm(manual),always_skip(hotfix/tweak)max_retry+ cross-phasemax_consecutive_failureswith exponential backoff.comet/auto/.activemarker — process-independent, multi-change safecomet autoCLI: Interactive config, validation, dry-run preview, cleanupFiles Changed
hooks/(session-start + 6 platform configs),comet-autoskill (zh/en),src/commands/auto.tsmanifest.json,comet-yaml-validate.sh,package.json(0.3.3 → 0.4.0), CLI indexTesting
All 159 existing tests pass with zero regressions.
Design Review
This feature went through 3 rounds of Claude review: 7.0 → 7.5 → 8.5. Full review notes in the handoff directory.
Configuration
Users create
comet-auto.yamlin their project root to enable auto-pilot. Change-level overrides available in.comet.yamlauto:field. Seecomet auto --initfor interactive setup.