jackwener / jackwener/OpenCLI

fix(douyin): fast_detect retry is driven by error-message substrings, so it breaks silently when wording changes

Open
#2,319 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
29.3k
Forks
2.9k
Avg merge
15h 36m
Merged PRs (30d)
70

Description

## Problem

`clis/douyin/publish.js` decides whether to retry the `fast_detect` safety check by **substring-matching the error message**:

```js
function isFastDetectRetryable(error) {
const message = error instanceof Error ? error.message : String(error);
return message.includes('post_assistant/fast_detect') && (message.includes('Empty response')
|| message.includes('404') || message.includes('Not Found') || message.includes('Timeout')
|| message.includes('timed out') || message.includes('Failed to fetch'));
}
```

There is no type-based fallback: `.code ===` / `errorCode` appear **0 times** in the whole file. Verified live on current `main`.

## Why this matters

This already failed once, silently. Before #2254, an empty `200` from the Douyin API surfaced as `"JSON parse failed: Unexpected end of JSON input"`, which does **not** contain `'Empty response'` — so this retry was dead in practice and nobody noticed. #2254 changed the message and incidentally revived it. That means the retry's liveness currently depends on upstream error *wording* staying byte-compatible, which nothing enforces.

The failure mode is the worst kind: rewording an error string anywhere upstream silently disconnects a safety-check retry on a **write** path, and no test fails.

## Why tests don't catch it

Ordinary behavioural tests assert only that *an error was thrown*, not *which class it was*. Type degradation is invisible to them. This same "error classification flattened across layers" pattern showed up three times in the recent adapter review round:
- here (still live on `main`)
- `clis/boss/detail.js` swallowing `AuthRequiredError` behind a catch-all (fixed in #2291)
- #2256 mislabelling a post-click write failure as a definite failure (fixed before merge)

## Suggested fix

1. Drive the retry decision off the typed error's `code` (and `phase` where relevant) instead of message text; keep substring matching only as a last-resort fallback, if at all.
2. **Tests must assert the typed error's `code`/`phase` directly**, not merely that a rejection occurred — otherwise the fix ships without regression protection and can rot the same way. Per @codex-coder, the final write click is the right pre/post boundary for that assertion.

## Context

Found during the task #304 lane B review (#2254 / #2291). Filing so it isn't lost: it is the only one of the three occurrences that is still live on `main`.

Contributor guide

Open the contributing guide

Research direction

Start in clis/douyin/publish.js and trace how the fast_detect error is created and how the final write click handles the pre/post boundary. Run the existing Douyin publish tests, then add coverage that asserts the typed error code and phase directly. Done means retry classification no longer depends primarily on error-message wording and the regression test verifies the typed fields.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
cli
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.