fix(import-markdown): CI測試登記 + .md目錄skip保護 (#588)#589
fix(import-markdown): CI測試登記 + .md目錄skip保護 (#588)#589jlin53882 wants to merge 5 commits intoCortexReach:masterfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a9c2e7091
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { group: "core-regression", runner: "node", file: "test/strip-envelope-metadata.test.mjs", args: ["--test"] }, | ||
| { group: "cli-smoke", runner: "node", file: "test/cli-smoke.mjs" }, | ||
| { group: "cli-smoke", runner: "node", file: "test/functional-e2e.mjs" }, | ||
| { group: "cli-smoke", runner: "node", file: "test/import-markdown/import-markdown.test.mjs", args: ["--test"] }, |
There was a problem hiding this comment.
Keep CI baseline verifier in sync with manifest changes
Adding test/import-markdown/import-markdown.test.mjs to CI_TEST_MANIFEST breaks the repo’s test entrypoint because scripts/verify-ci-test-manifest.mjs enforces an exact baseline list/order and does not include this new file. In this state, node scripts/verify-ci-test-manifest.mjs (and therefore npm test) fails immediately with unexpected manifest entry, so CI cannot run the intended test suite.
Useful? React with 👍 / 👎.
PR #589 CI cli-smoke 失敗說明cli-smoke 測試失敗不是本 PR 造成的,是 pre-existing 問題(源於 PR #582)。 失敗現象
根因分析PR #582(「skip 75ms retry when store is empty」)在 // src/tools.ts:178-182
let results = await retriever.retrieve(params);
if (results.length === 0) {
if (countStore) {
const total = await countStore();
if (total === 0) return results; // ← 這裡直接返回,不等第二次 retry
}
await sleep(75);
results = await retriever.retrieve(params);
}cli-smoke.mjs 的 mock store: {
async patchMetadata() {},
// count() 故意不回傳任何值,或回傳 0
}這導致即使 retriever 的第二次呼叫會回傳結果,邏輯也會在第一次就短路返回,第二次 retrieve 永遠不會執行。 驗證方式# 在 upstream/master(無本 PR)上同樣失敗
git checkout upstream/master
npm run test:cli-smoke
# → FAIL: CLI smoke test failed(與本 PR 的 CI 失敗完全相同)
# 本 PR 只改了 4 個檔案,cli-smoke.mjs 不在其中
git diff upstream/master -- test/cli-smoke.mjs
# → (無輸出,cli-smoke.mjs 完全未修改)修復建議cli-smoke.mjs 的 mock store: {
async patchMetadata() {},
async count() { return 1; }, // 新增:讓 PR #582 的優化邏輯不要觸發
}本 PR 內容(均無問題)
本 PR 解決了 Issue #588 的兩個問題,CI 中 cli-smoke 失敗為既有問題,與本 PR 無關。建議 maintainer 另開 Issue 追蹤 cli-smoke mock 更新。 |
rwmjhb
left a comment
There was a problem hiding this comment.
Both fixes here (missing CI entry + EISDIR crash on .md-named directories) are valid. The CI wiring issue exists in this PR too.
Must fix before merge:
-
MR2: The new CI manifest entry does not actually execute. The entry is placed after the cli-smoke test, which is currently failing on every CI run. The new import-markdown tests will never run until the manifest ordering is fixed or the smoke test is unblocked.
-
F2:
foundFilesis incremented before the stat/isDirectorycheck. Directories with.md-like names inflate the found count, which could mask zero-file scenarios or produce misleading output. -
MR1: Broad
catchturns genuine file-read failures into skippable warnings. A real I/O error (permissions, corruption) will be silently swallowed as awarn. The directory-skip path should be scoped specifically toEISDIR, not all errors.
Nice to have (non-blocking):
- F1: The new test has a dead assertion and a vacuously-true check that won't catch regressions.
- EF3: Two
console.warnadditions in the production code path — consider respecting a quiet-mode flag.
Fix the three items above and this is ready.
- cli.ts: 讀檔前先stat確認isFile(),非檔案skip並log warn - ci-test-manifest.mjs: 加入import-markdown.test.mjs到cli-smoke群組 - import-markdown.test.mjs: 新增測試『skip .md directory』 Ref: PR CortexReach#482 issuecomment-4229349438 (app3apps)
1a83d72 to
dd7f70b
Compare
Addressing rwmjhb's CHANGES_REQUESTED review: - MR1: catch 改為僅處理 EISDIR,其他 I/O 錯誤(權限、損壞)重新拋出 - F2: foundFiles++ 移至 stat + isFile() 檢查之後,確保只計入實際檔案 - MR2: import-markdown CI entry 移至 cli-smoke 之前,確保 cli-smoke 失敗時仍會執行 - F1: 修復 vacuously-true 斷言 assert.ok(skipped >= 0) → assert.strictEqual(skipped, 1)
Review 修正完成 (dc92a17)已處理所有 CHANGES_REQUESTED 項目:
Non-blocking 備註(EF3): Rebase 後 base 已更新至 |
…m manifest update" This reverts commit 8b8f2f5.
The mock store in cli-smoke.mjs was missing the async count() method that the real store interface provides. This caused the assertion at line ~316 to fail with undefined !== 1 when recallResult.details.count was accessed. This is a pre-existing issue introduced by the lifecycle-aware memory decay commit, not related to PR CortexReach#516.
Summary
Follow-up to PR #482, addressing two issues identified by maintainer @app3apps:
Issue #588: #588
Fix 1:CI manifest 沒有 import-markdown 測試
test/import-markdown/import-markdown.test.mjs沒有被加進 CI manifest,所以npm run test不會跑這個測試。修復:
scripts/ci-test-manifest.mjs— 在cli-smoke群組加入 entry:Fix 2:
.md目錄讓 import abort當
memory/下有一個實際是目錄但命名像.md檔案(例如2026-04-11.md資料夾),readdir會把它列出來,f.endsWith('.md')會通過,後續readFile拋EISDIR錯誤導致整個 import abort。修復:
cli.ts— 讀每個.md檔前先 stat 確認isFile(),非檔案就 skip 並 log warn:新增測試
test/import-markdown/import-markdown.test.mjs— 新增「skip non-file .md entries」測試:memory/2026-04-12.md為目錄(不是檔案)memory/2026-04-11.md測試結果:pass 13 / fail 0
Ref: app3apps 在 PR #482 issuecomment-4229349438 的 comment
Closes: Issue #588