starred/cli-continues

Fork 0

mirror of https://github.com/yigitkonur/cli-continues.git synced 2026-04-26 00:36:02 +03:00

[PR #32] fix(qwen-code): address pr #27 review comments #32

New issue

Closed

opened 2026-03-04 01:39:21 +03:00 by kerem · 0 comments

kerem commented

2026-03-04 01:39:21 +03:00

Owner

Original Pull Request: https://github.com/yigitkonur/cli-continues/pull/32

State: closed
Merged: Yes

addresses all 7 review comments from #27

zod schema validation instead of unsafe as casts (#27 review by greptile)
defensive timestamp parsing with fs.stat fallback (#27 review by copilot)
deduplicate tool_result vs functionCall double-counting (#27 review by copilot)
reconstruct main conversation path from uuid/parentUuid tree (#27 review by copilot)
rename antigravity fixture .json → .jsonl for accuracy (#27 review by copilot)

all 694 tests pass, build clean.

cc @karatechopping @copilot-pull-request-reviewer

Review all of them with eye of John Carmack-like simplicity with elegeance approach and apply the one only if required

Greptile Summary

This PR addresses 5 of 7 review comments from #27: moves Qwen Code interface definitions into Zod schemas in src/types/schemas.ts, wires up safeParse validation throughout the parser, adds a defensive parseTimestamp helper, deduplicates tool_result vs functionCall double-counting via a UUID set, reconstructs the main conversation path from the uuid/parentUuid tree, and renames the Antigravity fixture to .jsonl.

Changes:

src/types/schemas.ts — New QwenPartSchema, QwenContentSchema, QwenFileDiffSchema, QwenToolCallResultSchema, QwenUsageMetadataSchema, QwenChatRecordSchema with correct .passthrough() on all schemas.
src/parsers/qwen-code.ts — Removes all inline interface definitions and replaces unsafe as casts with QwenChatRecordSchema.safeParse(); adds reconstructMainPath() for proper branching conversation support; adds processedCallUuids dedup set.
src/__tests__/fixtures/index.ts — Fixture file renamed from session.json → session.jsonl (safe — test and parser both already accept both extensions).

Issues found:

Bug: reconstructMainPath() has no cycle detection in its while loop. A corrupted JSONL file with a circular parentUuid reference will hang the CLI indefinitely. Fix: track visited UUIDs in a Set.
Nit: Third branch of QwenToolCallResultSchema's resultDisplay union (z.record(z.string(), z.unknown())) is unreachable dead code because QwenFileDiffSchema (all-optional passthrough) matches every object first.

Confidence Score: 3/5

Safe to merge after fixing the cycle-detection bug in reconstructMainPath — without it a crafted or corrupted session file can permanently hang the CLI.
All the stated goals (Zod validation, dedup, timestamp fallback, fixture rename) are implemented correctly. One confirmed logic bug remains: reconstructMainPath() will infinite-loop on cyclic parentUuid data, violating the parser fault-tolerance contract. The fix is a one-liner (visited Set). Everything else is sound.
src/parsers/qwen-code.ts — specifically the reconstructMainPath() while loop (lines 413-417).

Important Files Changed

Filename	Overview
src/parsers/qwen-code.ts	Good: Zod validation added, types moved to schemas.ts, deduplication via processedCallUuids, parseTimestamp fallback. Bug: reconstructMainPath() has no cycle detection — cyclic parentUuid corrupts a session file will hang the process forever.
src/types/schemas.ts	Clean Zod schemas for all Qwen types with correct .passthrough() usage. Minor issue: QwenToolCallResultSchema's third union branch (z.record) is unreachable since QwenFileDiffSchema is all-optional passthrough and matches any object.
src/tests/fixtures/index.ts	Trivial rename of Antigravity fixture from session.json to session.jsonl. Correct — the antigravity parser already accepts both .json and .jsonl, and the test lookup already uses .endsWith('.json')

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[JSONL line read] --> B{JSON.parse}
    B -- throws --> C[catch: debug log, skip]
    B -- ok --> D{QwenChatRecordSchema.safeParse}
    D -- failure --> E[silent skip]
    D -- success --> F[QwenChatRecord]

    F --> G{record.type}
    G -- assistant + functionCall parts --> H[processedCallUuids.add uuid]
    H --> I[collector.add per tool category]
    G -- tool_result --> J{parentUuid in processedCallUuids?}
    J -- yes --> K[skip - dedup]
    J -- no --> L[collector.add file diff]

    subgraph reconstructMainPath
        M[Build byUuid map + parentUuids set] --> N[Find latest leaf node]
        N --> O["Walk parentUuid chain back to root (⚠️ no cycle guard)"]
        O --> P[Ordered main conversation path]
    end

    F --> reconstructMainPath
    P --> Q[Filter user/assistant records]
    Q --> R[generateHandoffMarkdown]

_{Last reviewed commit: c533789}

**Original Pull Request:** https://github.com/yigitkonur/cli-continues/pull/32 **State:** closed **Merged:** Yes --- addresses all 7 review comments from #27 - zod schema validation instead of unsafe `as` casts (#27 review by greptile) - defensive timestamp parsing with fs.stat fallback (#27 review by copilot) - deduplicate tool_result vs functionCall double-counting (#27 review by copilot) - reconstruct main conversation path from uuid/parentUuid tree (#27 review by copilot) - rename antigravity fixture .json → .jsonl for accuracy (#27 review by copilot) all 694 tests pass, build clean. cc @karatechopping @copilot-pull-request-reviewer  --- <a href="https://app.devin.ai/review/yigitkonur/cli-continues/pull/32" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a>   # Review all of them with eye of John Carmack-like simplicity with elegeance approach and apply the one only if required <h3>Greptile Summary</h3> This PR addresses 5 of 7 review comments from #27: moves Qwen Code interface definitions into Zod schemas in `src/types/schemas.ts`, wires up `safeParse` validation throughout the parser, adds a defensive `parseTimestamp` helper, deduplicates `tool_result` vs `functionCall` double-counting via a UUID set, reconstructs the main conversation path from the `uuid`/`parentUuid` tree, and renames the Antigravity fixture to `.jsonl`. **Changes:** - `src/types/schemas.ts` — New `QwenPartSchema`, `QwenContentSchema`, `QwenFileDiffSchema`, `QwenToolCallResultSchema`, `QwenUsageMetadataSchema`, `QwenChatRecordSchema` with correct `.passthrough()` on all schemas. - `src/parsers/qwen-code.ts` — Removes all inline `interface` definitions and replaces unsafe `as` casts with `QwenChatRecordSchema.safeParse()`; adds `reconstructMainPath()` for proper branching conversation support; adds `processedCallUuids` dedup set. - `src/__tests__/fixtures/index.ts` — Fixture file renamed from `session.json` → `session.jsonl` (safe — test and parser both already accept both extensions). **Issues found:** - **Bug:** `reconstructMainPath()` has no cycle detection in its `while` loop. A corrupted JSONL file with a circular `parentUuid` reference will hang the CLI indefinitely. Fix: track visited UUIDs in a `Set`. - **Nit:** Third branch of `QwenToolCallResultSchema`'s `resultDisplay` union (`z.record(z.string(), z.unknown())`) is unreachable dead code because `QwenFileDiffSchema` (all-optional passthrough) matches every object first. <details><summary><h3>Confidence Score: 3/5</h3></summary> - Safe to merge after fixing the cycle-detection bug in reconstructMainPath — without it a crafted or corrupted session file can permanently hang the CLI. - All the stated goals (Zod validation, dedup, timestamp fallback, fixture rename) are implemented correctly. One confirmed logic bug remains: reconstructMainPath() will infinite-loop on cyclic parentUuid data, violating the parser fault-tolerance contract. The fix is a one-liner (visited Set). Everything else is sound. - src/parsers/qwen-code.ts — specifically the reconstructMainPath() while loop (lines 413-417). </details> <details open><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | src/parsers/qwen-code.ts | Good: Zod validation added, types moved to schemas.ts, deduplication via processedCallUuids, parseTimestamp fallback. Bug: reconstructMainPath() has no cycle detection — cyclic parentUuid corrupts a session file will hang the process forever. | | src/types/schemas.ts | Clean Zod schemas for all Qwen types with correct .passthrough() usage. Minor issue: QwenToolCallResultSchema's third union branch (z.record) is unreachable since QwenFileDiffSchema is all-optional passthrough and matches any object. | | src/__tests__/fixtures/index.ts | Trivial rename of Antigravity fixture from session.json to session.jsonl. Correct — the antigravity parser already accepts both .json and .jsonl, and the test lookup already uses .endsWith('.json') || .endsWith('.jsonl'). | </details> </details> <h3>Flowchart</h3> ```mermaid %%{init: {'theme': 'neutral'}}%% flowchart TD A[JSONL line read] --> B{JSON.parse} B -- throws --> C[catch: debug log, skip] B -- ok --> D{QwenChatRecordSchema.safeParse} D -- failure --> E[silent skip] D -- success --> F[QwenChatRecord] F --> G{record.type} G -- assistant + functionCall parts --> H[processedCallUuids.add uuid] H --> I[collector.add per tool category] G -- tool_result --> J{parentUuid in processedCallUuids?} J -- yes --> K[skip - dedup] J -- no --> L[collector.add file diff] subgraph reconstructMainPath M[Build byUuid map + parentUuids set] --> N[Find latest leaf node] N --> O["Walk parentUuid chain back to root (⚠️ no cycle guard)"] O --> P[Ordered main conversation path] end F --> reconstructMainPath P --> Q[Filter user/assistant records] Q --> R[generateHandoffMarkdown] ``` <sub>Last reviewed commit: c533789</sub>

kerem

2026-03-04 01:39:21 +03:00

closed this issue
added the
pull-request
label

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/cli-continues#32

No description provided.

Rows
Columns