fix: add evidence format command and narrow ENOTDIR handling#228
Open
1919chichi wants to merge 15 commits into
rpamis:masterrpamis/comet:masterfrom
1919chichi:fix/native-evidence-format-and-fs-notdir1919chichi/comet:fix/native-evidence-format-and-fs-notdirCopy head branch name to clipboard
Open
fix: add evidence format command and narrow ENOTDIR handling#2281919chichi wants to merge 15 commits intorpamis:masterrpamis/comet:masterfrom 1919chichi:fix/native-evidence-format-and-fs-notdir1919chichi/comet:fix/native-evidence-format-and-fs-notdirCopy head branch name to clipboard
1919chichi wants to merge 15 commits into
rpamis:masterrpamis/comet:masterfrom
1919chichi:fix/native-evidence-format-and-fs-notdir1919chichi/comet:fix/native-evidence-format-and-fs-notdirCopy head branch name to clipboard
Conversation
Adds `comet native evidence format`, which serializes acceptance evidence entries into the exact canonical Markdown block verification.md requires. The parser in native-acceptance.ts compares the raw block byte-for-byte against a re-serialized canonical form (a deliberate defense against duplicate-key JSON smuggling), so hand-typed JSON almost never matches regardless of formatting style. This command removes the need to hand-format the block at all.
Updates both Chinese and English Native SKILL.md and reference docs (artifacts.md, commands.md) to direct agents to `comet native evidence format` instead of hand-typing the acceptance evidence JSON block.
fileExists() treats ENOTDIR the same as ENOENT so a stray file where a plugin marketplace directory is expected doesn't crash comet init. removeFile()/removeDir() had picked up the same broadened check, which would silently treat a removal hitting ENOTDIR as "already gone" instead of surfacing a real on-disk anomaly. Scopes the ENOTDIR handling back to fileExists only and adds regression coverage for both the original crash and the removal behavior.
Syncs package.json, package-lock.json, assets/manifest.json, and the version assertions in cli-help.test.ts. Adds the beta.9 changelog entry covering the evidence-format command and the plugin marketplace ENOTDIR crash fix.
Reviewer's GuideImplements a new Sequence diagram for the new comet native evidence format commandsequenceDiagram
actor User
participant NativeCLI as native_cli.dispatch
participant FS as fs
participant JSONParser as JSON
participant Acceptance as serializeNativeVerificationMachineBlock
participant Result as success
User->>NativeCLI: dispatch(rawArgs, explicitProjectRoot)
NativeCLI->>NativeCLI: requiredPositional(rawArgs, evidence subcommand)
NativeCLI->>NativeCLI: takeOption(rawArgs, --entries)
alt entriesPath provided
NativeCLI->>FS: fs.readFile(path.resolve(entriesPath), utf8)
FS-->>NativeCLI: raw
else stdin JSON
NativeCLI->>NativeCLI: process.stdin.isTTY check
NativeCLI-->>User: NativeUsageError
NativeCLI->>NativeCLI: readFileSync(0, utf8)
NativeCLI-->>NativeCLI: raw
end
NativeCLI->>JSONParser: JSON.parse(raw)
JSONParser-->>NativeCLI: entries
NativeCLI->>NativeCLI: Array.isArray(entries)
NativeCLI->>Acceptance: serializeNativeVerificationMachineBlock(entries)
Acceptance-->>NativeCLI: block
NativeCLI->>Result: success(evidence format, { block }, block + "\n")
Result-->>User: CLI output with canonical Markdown block
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesNative runtime and filesystem updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant NativeCLI
participant EvidenceSerializer
participant VerificationMd
Operator->>NativeCLI: Provide entries via stdin or --entries
NativeCLI->>EvidenceSerializer: Serialize entries into canonical marked block
EvidenceSerializer-->>NativeCLI: Return formatted verification Markdown
Operator->>VerificationMd: Paste generated acceptance evidence block
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 49-55: Move the 0.4.0-beta.9 changelog entry above all older
release entries, add an “Added” section containing the comet native evidence
format change, and keep the plugin marketplace superpowers detection bullet
under a separate “Fixed” section.
In `@domains/comet-native/native-cli.ts`:
- Line 1: Replace direct filesystem and stdin imports in
domains/comet-native/native-cli.ts with the platform adapter, and update the
evidence-loading flow around lines 477-506 to read through its bounded Native
evidence-size API before JSON.parse. Keep native-cli focused on parsing and
serialization. Regenerate
assets/skills/comet-native/scripts/comet-native-runtime.mjs so the generated
asset reflects the source changes at lines 7371 and 28700-28730; do not
hand-edit the bundled output.
In `@test/platform/detect.test.ts`:
- Around line 492-515: Wrap the test body that modifies
process.env.CLAUDE_CONFIG_DIR and creates the plugin fixture in a try/finally
block. Move the existing restoration logic for origEnv into finally so
CLAUDE_CONFIG_DIR is restored or deleted even when setup or
hasPluginSuperpowers() fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04c166c6-911c-47d3-b2ce-78387507cd1f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
CHANGELOG.mdassets/manifest.jsonassets/skills-zh/comet-native/SKILL.mdassets/skills-zh/comet-native/reference/artifacts.mdassets/skills-zh/comet-native/reference/commands.mdassets/skills/comet-native/SKILL.mdassets/skills/comet-native/reference/artifacts.mdassets/skills/comet-native/reference/commands.mdassets/skills/comet-native/scripts/comet-native-runtime.mjsassets/skills/comet/scripts/comet-runtime.mjsdomains/comet-native/native-cli.tspackage.jsonplatform/fs/file-system.tstest/app/cli-help.test.tstest/domains/comet-native/native-cli.test.tstest/platform/detect.test.tstest/platform/file-system.test.ts
Comment on lines
+492
to
+515
| const origEnv = process.env.CLAUDE_CONFIG_DIR; | ||
| const pluginDir = path.join(tmpDir, '.claude'); | ||
| process.env.CLAUDE_CONFIG_DIR = pluginDir; | ||
|
|
||
| const cacheDir = path.join(pluginDir, 'plugins', 'cache'); | ||
| await fs.mkdir(cacheDir, { recursive: true }); | ||
| // A stray file (e.g. macOS .DS_Store) sitting where a marketplace | ||
| // directory is expected used to make fs.access throw ENOTDIR and crash | ||
| // comet init instead of being skipped like a missing marketplace. | ||
| await fs.writeFile(path.join(cacheDir, '.DS_Store'), ''); | ||
|
|
||
| const skillsDir = path.join(cacheDir, 'test-marketplace', 'superpowers', '5.0.0', 'skills'); | ||
| await fs.mkdir(skillsDir, { recursive: true }); | ||
| await fs.mkdir(path.join(skillsDir, 'brainstorming')); | ||
| await fs.mkdir(path.join(skillsDir, 'using-superpowers')); | ||
|
|
||
| await expect(hasPluginSuperpowers()).resolves.toBe(true); | ||
|
|
||
| if (origEnv !== undefined) { | ||
| process.env.CLAUDE_CONFIG_DIR = origEnv; | ||
| } else { | ||
| delete process.env.CLAUDE_CONFIG_DIR; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore CLAUDE_CONFIG_DIR in a finally block.
If directory setup or the assertion fails, the cleanup at Lines 510-515 is skipped and the process-wide environment variable leaks into subsequent tests. Wrap the test body in try/finally.
Proposed fix
const origEnv = process.env.CLAUDE_CONFIG_DIR;
const pluginDir = path.join(tmpDir, '.claude');
process.env.CLAUDE_CONFIG_DIR = pluginDir;
- const cacheDir = path.join(pluginDir, 'plugins', 'cache');
- await fs.mkdir(cacheDir, { recursive: true });
- await fs.writeFile(path.join(cacheDir, '.DS_Store'), '');
-
- const skillsDir = path.join(cacheDir, 'test-marketplace', 'superpowers', '5.0.0', 'skills');
- await fs.mkdir(skillsDir, { recursive: true });
- await fs.mkdir(path.join(skillsDir, 'brainstorming'));
- await fs.mkdir(path.join(skillsDir, 'using-superpowers'));
-
- await expect(hasPluginSuperpowers()).resolves.toBe(true);
-
- if (origEnv !== undefined) {
- process.env.CLAUDE_CONFIG_DIR = origEnv;
- } else {
- delete process.env.CLAUDE_CONFIG_DIR;
+ try {
+ const cacheDir = path.join(pluginDir, 'plugins', 'cache');
+ await fs.mkdir(cacheDir, { recursive: true });
+ await fs.writeFile(path.join(cacheDir, '.DS_Store'), '');
+ const skillsDir = path.join(
+ cacheDir,
+ 'test-marketplace',
+ 'superpowers',
+ '5.0.0',
+ 'skills',
+ );
+ await fs.mkdir(skillsDir, { recursive: true });
+ await fs.mkdir(path.join(skillsDir, 'brainstorming'));
+ await fs.mkdir(path.join(skillsDir, 'using-superpowers'));
+ await expect(hasPluginSuperpowers()).resolves.toBe(true);
+ } finally {
+ if (origEnv !== undefined) {
+ process.env.CLAUDE_CONFIG_DIR = origEnv;
+ } else {
+ delete process.env.CLAUDE_CONFIG_DIR;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const origEnv = process.env.CLAUDE_CONFIG_DIR; | |
| const pluginDir = path.join(tmpDir, '.claude'); | |
| process.env.CLAUDE_CONFIG_DIR = pluginDir; | |
| const cacheDir = path.join(pluginDir, 'plugins', 'cache'); | |
| await fs.mkdir(cacheDir, { recursive: true }); | |
| // A stray file (e.g. macOS .DS_Store) sitting where a marketplace | |
| // directory is expected used to make fs.access throw ENOTDIR and crash | |
| // comet init instead of being skipped like a missing marketplace. | |
| await fs.writeFile(path.join(cacheDir, '.DS_Store'), ''); | |
| const skillsDir = path.join(cacheDir, 'test-marketplace', 'superpowers', '5.0.0', 'skills'); | |
| await fs.mkdir(skillsDir, { recursive: true }); | |
| await fs.mkdir(path.join(skillsDir, 'brainstorming')); | |
| await fs.mkdir(path.join(skillsDir, 'using-superpowers')); | |
| await expect(hasPluginSuperpowers()).resolves.toBe(true); | |
| if (origEnv !== undefined) { | |
| process.env.CLAUDE_CONFIG_DIR = origEnv; | |
| } else { | |
| delete process.env.CLAUDE_CONFIG_DIR; | |
| } | |
| }); | |
| const origEnv = process.env.CLAUDE_CONFIG_DIR; | |
| const pluginDir = path.join(tmpDir, '.claude'); | |
| process.env.CLAUDE_CONFIG_DIR = pluginDir; | |
| try { | |
| const cacheDir = path.join(pluginDir, 'plugins', 'cache'); | |
| await fs.mkdir(cacheDir, { recursive: true }); | |
| await fs.writeFile(path.join(cacheDir, '.DS_Store'), ''); | |
| const skillsDir = path.join(cacheDir, 'test-marketplace', 'superpowers', '5.0.0', 'skills'); | |
| await fs.mkdir(skillsDir, { recursive: true }); | |
| await fs.mkdir(path.join(skillsDir, 'brainstorming')); | |
| await fs.mkdir(path.join(skillsDir, 'using-superpowers')); | |
| await expect(hasPluginSuperpowers()).resolves.toBe(true); | |
| } finally { | |
| if (origEnv !== undefined) { | |
| process.env.CLAUDE_CONFIG_DIR = origEnv; | |
| } else { | |
| delete process.env.CLAUDE_CONFIG_DIR; | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 500-500: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(path.join(cacheDir, '.DS_Store'), '')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/platform/detect.test.ts` around lines 492 - 515, Wrap the test body that
modifies process.env.CLAUDE_CONFIG_DIR and creates the plugin fixture in a
try/finally block. Move the existing restoration logic for origEnv into finally
so CLAUDE_CONFIG_DIR is restored or deleted even when setup or
hasPluginSuperpowers() fails.
`comet native evidence format` read the whole --entries file or stdin into memory via fs.readFile/readFileSync before JSON.parse, with no size limit — unlike the rest of the Native evidence subsystem, which enforces explicit byte caps on evidence-shaped documents (native-verification-scope.ts, native-change.ts, native-run-store.ts). Adds readBoundedEvidenceFile (stat-then-read) and readBoundedEvidenceStdin (aborts mid-stream once the cap is exceeded), both bounded by the existing MAX_NATIVE_IMPLEMENTATION_EVIDENCE_DOCUMENT_BYTES constant, and regenerates comet-native-runtime.mjs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
The [0.4.0-beta.9] entry was inserted after beta.8/beta.7 instead of at the top, and comet native evidence format — a new command — was filed entirely under Fixed. Moves the entry to the top of the file and gives it an Added section for the new command, keeping the ENOTDIR and greeting-workflow fixes under Fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/domains/comet-native/native-cli.test.ts (1)
759-780: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an oversized-stdin regression test.
This test covers only
--entriesfiles. Add a piped stdin payload larger thanMAX_NATIVE_IMPLEMENTATION_EVIDENCE_DOCUMENT_BYTESand assert the same rejection, since stdin has separate mid-stream limit logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/domains/comet-native/native-cli.test.ts` around lines 759 - 780, Add a regression test alongside the existing oversized entries-file test that pipes a payload larger than MAX_NATIVE_IMPLEMENTATION_EVIDENCE_DOCUMENT_BYTES into the native CLI evidence formatting command via stdin. Invoke runNativeCli with the stdin input and assert exit code 65 plus the same invalid-data error containing “exceeds”, covering the separate stdin size-limit path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@domains/comet-native/native-cli.ts`:
- Around line 190-195: Update readBoundedEvidenceFile to enforce the limit
during reading rather than relying on the initial fs.stat check: open the file
and read through the descriptor or stream with a hard maxBytes + 1 bound, then
reject when more than maxBytes is obtained. Preserve the existing success result
for files within the limit and the descriptive size-limit error for oversized
input.
---
Nitpick comments:
In `@test/domains/comet-native/native-cli.test.ts`:
- Around line 759-780: Add a regression test alongside the existing oversized
entries-file test that pipes a payload larger than
MAX_NATIVE_IMPLEMENTATION_EVIDENCE_DOCUMENT_BYTES into the native CLI evidence
formatting command via stdin. Invoke runNativeCli with the stdin input and
assert exit code 65 plus the same invalid-data error containing “exceeds”,
covering the separate stdin size-limit path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1879cdcc-1793-4fc7-aaa8-fa948e7f514e
📒 Files selected for processing (4)
CHANGELOG.mdassets/skills/comet-native/scripts/comet-native-runtime.mjsdomains/comet-native/native-cli.tstest/domains/comet-native/native-cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- assets/skills/comet-native/scripts/comet-native-runtime.mjs
Contributor
|
还有2项需要处理
|
- Replace fs.stat + fs.readFile with fs.open + bounded chunked reads using non-blocking flags so special files (e.g. FIFOs) cannot hang the evidence format command - Reject non-regular-file paths for --entries with a clear error - Sync generated comet-native-runtime.mjs and add FIFO regression test - Remove unrelated changelog entry not part of this branch
readCometCurrentSelection checked .comet/current-change.json with a path-based lstat/size check and then reopened the same path with readFile. Between the two calls the file could be grown past the 16 KiB limit or swapped for a symlink, bypassing both the byte bound and the regular-file check. This path is reachable from the Hook Router (every hooked tool call), comet doctor, comet resume-probe, and both Native and Classic change selection. Mirror the fd-based bounded read already used by readBoundedEvidenceFile in native-cli.ts: open once with O_NOFOLLOW | O_NONBLOCK, fstat the handle, and read through the same descriptor with a hard maxBytes + 1 bound, so the check and the read observe the same file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/skills/comet-native/scripts/comet-native-runtime.mjs (1)
28441-28479: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
readBoundedEvidenceFileopens withoutO_NOFOLLOW, unlike every sibling reader touched in this diff.
readCometCurrentSelection(line ~21477),native-evidence-retention.ts'sreadCanonicalDocument(line ~24223), andnative-check-receipt.ts'sreadScopedFile(line ~26548) all pairO_NONBLOCKwithO_NOFOLLOWon non-win32. This new helper forcomet native evidence format --entries <path>only setsO_NONBLOCK, so a symlink at the given path is silently followed. Given the CLI accepts an arbitrary path argument, this reintroduces the symlink-following class of issue the rest of the PR is fixing (e.g. an attacker-writable shared/tmp directory could substitute a symlink before the command runs).This is a generated asset; the fix belongs in
domains/comet-native/native-cli.tsand must be resynced via the build.🔒 Proposed fix (apply in domains/comet-native/native-cli.ts, then rebuild)
- const flags = process.platform === "win32" ? "r" : fsConstants7.O_RDONLY | fsConstants7.O_NONBLOCK; - const handle = await fs33.open(filePath, flags); + const flags = process.platform === "win32" ? "r" : fsConstants7.O_RDONLY | fsConstants7.O_NOFOLLOW | fsConstants7.O_NONBLOCK; + const handle = await fs33.open(filePath, flags).catch((error) => { + if (error.code === "ELOOP") { + throw new Error(`Acceptance evidence entries path must be a regular file: ${filePath}`, { cause: error }); + } + throw error; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-native/scripts/comet-native-runtime.mjs` around lines 28441 - 28479, Update readBoundedEvidenceFile to include O_NOFOLLOW alongside O_NONBLOCK in the non-Windows open flags, matching the sibling readers’ symlink protection. Apply the source change in native-cli.ts, then rebuild to resynchronize the generated comet-native-runtime.mjs asset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@assets/skills/comet-native/scripts/comet-native-runtime.mjs`:
- Around line 28441-28479: Update readBoundedEvidenceFile to include O_NOFOLLOW
alongside O_NONBLOCK in the non-Windows open flags, matching the sibling
readers’ symlink protection. Apply the source change in native-cli.ts, then
rebuild to resynchronize the generated comet-native-runtime.mjs asset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e09fae8a-61ce-4b37-9bdd-7445ee9c7359
📒 Files selected for processing (6)
CHANGELOG.mdassets/skills/comet-native/scripts/comet-native-runtime.mjsassets/skills/comet/scripts/comet-hook-router.mjsassets/skills/comet/scripts/comet-runtime.mjsdomains/comet-entry/current-selection.tstest/domains/comet-entry/current-selection.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Contributor
Author
Contributor
|
还有一些问题需要处理 native-cli.ts:190 的 readBoundedEvidenceFile 在非 Windows 也漏了 O_NOFOLLOW，因此 --entries 会跟随链接并把目标当作普通文件。这正是最新 CodeRabbit 尚未处理的评论。建议同时补 symlink 回归测试，并重新生成 Native runtime。 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
… relying on O_NOFOLLOW Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
…id-read swaps Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
…pective Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuxYieafr2jRwiL6KzPs43
Contributor
Author
|
@benym 两个问题都已在最新推送中修复( 你指出的两处读取都可能在检查和读取之间被调包—— 这三处读取现在统一走新增的共享读取函数 防护不再依赖 仓库里还有 6 处文件( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
domains/comet-native/native-lock.ts (1)
161-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap
JSON.parseso a malformed lock file yields a descriptive error.
JSON.parse(bytes.toString('utf8'))sits outside thetry/catch, so a corrupt-but-regular lock file throws a rawSyntaxErrorinstead of theInvalid Native lock metadata: <file>message thatparseNativeLockOwnergives for structural problems. Consider parsing inside a guarded block for consistent diagnostics.♻️ Suggested guard
return { file, - owner: parseNativeLockOwner(JSON.parse(bytes.toString('utf8')) as unknown, file), + owner: parseNativeLockOwner(parseLockJson(bytes.toString('utf8'), file), file), identity: nativeLockFileIdentity(stat), };where
parseLockJsoncatchesSyntaxErrorand rethrowsInvalid Native lock metadata: ${file}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domains/comet-native/native-lock.ts` around lines 161 - 165, Move the JSON.parse operation in the native lock parsing flow into the existing guarded try/catch, such as through the parseLockJson helper, so malformed lock contents are caught and rethrown with the same “Invalid Native lock metadata: <file>” diagnostic used by parseNativeLockOwner. Preserve the existing owner and identity construction for valid lock files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@domains/comet-native/native-lock.ts`:
- Around line 161-165: Move the JSON.parse operation in the native lock parsing
flow into the existing guarded try/catch, such as through the parseLockJson
helper, so malformed lock contents are caught and rethrown with the same
“Invalid Native lock metadata: <file>” diagnostic used by parseNativeLockOwner.
Preserve the existing owner and identity construction for valid lock files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db5bedc4-5268-487d-8f46-8453f2e49e98
📒 Files selected for processing (15)
CHANGELOG.mdassets/skills/comet-native/scripts/comet-native-runtime.mjsassets/skills/comet/scripts/comet-entry-runtime.mjsassets/skills/comet/scripts/comet-hook-router.mjsassets/skills/comet/scripts/comet-runtime.mjsdomains/comet-entry/current-selection.tsdomains/comet-native/native-cli.tsdomains/comet-native/native-file-identity.tsdomains/comet-native/native-lock.tsplatform/fs/file-identity.tsplatform/fs/race-safe-read.tstest/domains/comet-entry/current-selection.test.tstest/domains/comet-native/native-cli.test.tstest/domains/comet-native/native-lock.test.tstest/platform/race-safe-read.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- test/domains/comet-entry/current-selection.test.ts
- assets/skills/comet/scripts/comet-hook-router.mjs
- assets/skills/comet/scripts/comet-runtime.mjs
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
2 participants
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
comet native evidence format, which serializes acceptance evidence entries into the exact canonical Markdown blockverification.mdrequires. The parser does a byte-for-byte comparison against a re-serialized canonical form (a deliberate defense against duplicate-key JSON smuggling — seenative-acceptance.ts), so hand-typed JSON almost never matches regardless of formatting style. This command removes that friction entirely.ENOTDIRhandling that was added for the plugin-marketplace crash fix (a stray file, e.g..DS_Store, where a marketplace directory is expected) so it only applies tofileExists.removeFile/removeDirhad picked up the same broadened check, which would have silently treated a removal hittingENOTDIRas "already gone" instead of surfacing a real on-disk anomaly.SKILL.md,reference/artifacts.md,reference/commands.md) to point at the new command instead of describing hand-typed JSON.0.4.0-beta.9and updatesCHANGELOG.mdwith both fixes under the same entry.This PR bundles two fixes that would otherwise be separate contributions. Splitting them would require one to land first and claim
beta.9while the other rebases tobeta.10, so they're kept together in one version bump perCONTRIBUTING.md's versioning rule.Review follow-ups
evidence formatnow reads--entries <path>/stdin throughreadBoundedEvidenceFile/readBoundedEvidenceStdininnative-cli.ts, capped at the existingMAX_NATIVE_IMPLEMENTATION_EVIDENCE_DOCUMENT_BYTES(1 MB) used elsewhere for Native evidence documents, instead of buffering an unbounded read beforeJSON.parse.comet-native-runtime.mjsis regenerated to match.[0.4.0-beta.9]entry is moved to the top ofCHANGELOG.md(it had landed after beta.8/beta.7) andcomet native evidence formatis filed under a new### Addedsection instead of### Fixed, since it's a new command rather than a fix to previously broken behavior.native-cli.ts's file/stdin reads through aplatform/adapter.domains/comet-nativeanddomains/comet-classicalready importfsdirectly in 30+ files (including stdin reads inclassic-hook-guard.ts), andlint:architecturedoesn't restrict this —platform/in this repo owns platform-difference handling (e.g. theENOTDIRfix above), not "no domain file may callfs". Happy to revisit if maintainers want that convention formalized.🎯 Scope
init,status,doctor,update)assets/skills/,assets/skills-zh/)assets/skills/comet/scripts/)comet nativeCLI (newevidence formatsubcommand)🧪 Testing
pnpm buildpnpm lintpnpm run lint:architecturepnpm format:checkpnpm testpnpm test -- test/domains/comet-classic/comet-scripts.test.ts— covered by the fullpnpm testrun aboveReview follow-up commits (bounded evidence input, CHANGELOG reorder) were validated at the scope the changes touched, per this repo's risk-matched testing convention, rather than re-running the full suite:
pnpm build:native-runtime(regeneratescomet-native-runtime.mjs) +npx vitest run test/repository/native-runtime-assets.test.tsnpx vitest run test/domains/comet-native/native-cli.test.tsand the fulltest/domains/comet-nativedirectory — same 2 pre-existing TOCTOU failures as before, see Notes belownpx eslint/npx prettier --checkon the changed files,node scripts/lint/architecture.mjs✅ Checklist
fix: handle project-scope initREADME.md,README-zh.md, orCONTRIBUTING.md— recorded inCHANGELOG.mdper this repo's README-restraint conventionCHANGELOG.mdis updated when behavior changesassets/manifest.jsonand relevant tests — no new scripts;assets/manifest.json's version was synced.mjsbundles👀 Notes for Reviewers
pnpm testhas 2 pre-existing failures unrelated to this PR:native-check-receipt.test.ts("detects a same-size replacement between lstat and open...") andnative-snapshot.test.ts("rejects a file identity swap..."). Both are timing-sensitive TOCTOU file-identity tests; I reproduced the identical failures on an unmodifiedorigin/mastercheckout in a separate worktree, so they're not a regression from this branch. They still reproduce identically after the review follow-up commits.ENOTDIRscoping fix (platform/fs/file-system.ts) is verified with a regression test intest/platform/detect.test.tsthat reproduces the original crash shape (a stray file where a plugin marketplace directory is expected), plus two new tests intest/platform/file-system.test.tsprovingremoveFile/removeDirnow propagateENOTDIRinstead of masking it.try/finallyenv-var cleanup in the newtest/platform/detect.test.tscase — noted, but left as-is for now since it follows the exact same pattern as the pre-existing neighboring test in that file; happy to fix both together in a follow-up if maintainers want it.Summary by Sourcery
Introduce a native CLI subcommand for canonical acceptance evidence formatting and tighten filesystem error handling around plugin marketplace detection and removal helpers.
New Features:
comet native evidence formatsubcommand to serialize acceptance evidence entries into the canonical Markdown block required by verification docs.Bug Fixes:
comet initfrom crashing when the plugin marketplace cache contains stray non-directory files by treating ENOTDIR as a missing path only in existence checks.removeFileandremoveDirpropagate ENOTDIR errors instead of masking them as already-removed paths.Enhancements:
Build:
Documentation:
comet native evidence formatusage in native skill references and SKILL guides in both English and Chinese.Tests:
Summary by CodeRabbit
comet native evidence formatto generate canonicalverification.mdacceptance-evidence blocks from JSON (stdin or--entries).comet initcrashing withENOTDIRwhen stray files exist in the plugin cache path.ENOENT/ENOTDIR) and updated removal behavior forENOTDIR.