[codex] unify CLI progress with engine snapshot

[breezeFur]

Closes #97

Summary

The CLI now drives its progress display from the same Engine.snapshot() contract used by the GUI.

What changed

• Added thread-safe external progress transitions to Engine for a front-end-owned asyncio run.
• Removed the CLI's duplicate byte-rate arithmetic, counters and lock-protected display state.
• Added explicit extracting and downloading phases with completed/total counts.
• Suppressed identical progress snapshots so slow extraction does not print repeated zero-speed lines.
• Added CLI progress contract tests and documented the output.

The CLI still owns its existing queue and extraction/download scheduling. The engine is used as the progress snapshot owner, which keeps the change focused while making future speed or ETA fixes apply to both front-ends.

Review follow-up

Extraction progress is now keyed by URL inside Engine.mark_extraction(). A stall-killed URL can be re-extracted without incrementing extract_done a second time, so the numerator cannot pass extract_total or end the extraction stage early. I chose unique-URL accounting in the engine instead of guarding individual CLI call sites because it keeps the snapshot invariant at the boundary that owns the counter.

The branch was also rebased over the structured CLI exit-code change from #153. The final return path now reads the successful and failed counts from the engine snapshot metrics, preserving those exit codes without restoring duplicate CLI counters.

Validation

• python -m pytest tests/ -q -> 53 passed
• python -m pytest tests/test_cli_exit_codes.py tests/test_cli_progress.py tests/test_no_chrome.py -q -> 19 passed
• ruff check .
• git diff --check
• GitHub Actions -> 8 checks passed

No new dependencies were added. Prepared with Codex assistance; the diff and tests were reviewed locally before push.

[XEDAB]

this is cleaner than my own approach but mark_extraction(True) bumps _url_done on every successful extraction,a stall-killed URL gets re-queued and re-extracted, so the same URL can bump it more than once. once _url_done >= _url_total, stage leaves extracting early while retries are still in flight. Should this count unique URLs rather than extraction attempts?

[XEDAB]

also, I noticed this PR's title is unusual — its prefix is [codex].

[LeyckerS]

@breezeFur — the shape of this is right. Keeping the CLI's own queue and scheduling and using the engine only as the progress owner is the version of #97 I was hoping for; the alternative, routing the whole run through Engine.start(), was tried on the same day and hits problems this approach avoids entirely.

@XEDAB's review comment is correct and it blocks. I verified it against the source rather than taking either of your words for it.

mark_extraction() bumps _url_done unconditionally:

def mark_extraction(self, success: bool):
    with self._lock:
        self._url_done += 1

Your two success call sites are not guarded, while the failure site is:

progress.mark_extraction(True)          # ← both success paths, unguarded
...
if not success and not is_re and not fatal_control.is_set():
    ...
    if progress.mark_extraction(False): # ← guarded by `not is_re`

and moon_cli.py:196 defines is_re = rec.stall_kills > 0, with the re-queue at moon_cli.py:167. So a stall-killed URL comes back through the worker, extracts successfully a second time, and increments _url_done again for the same URL. _url_total is len(urls), one per URL, so the counter is now measuring a different quantity from its denominator.

The consequence @XEDAB describes is real, at moon_engine.py:743:

elif url_done < url_tot:
    stage = "extracting"

Once the double-counting pushes url_done to url_tot, stage leaves extracting while re-extractions are still in flight, and your extracting {extract_done}/{extract_total} line can print a numerator past its denominator. On a run with stall kills — which is the run this project has most — that is visible.

Counting unique URLs is the right fix, as they suggested. Guarding the success calls with not is_re would also work and is smaller; either is fine, but say which you chose and why in the body.

On the [codex] prefix, since @XEDAB raised it: it is not a problem and no one needs to hide it. This repository's own commits carry Co-Authored-By: Claude, and #150 — the most careful pull request this project has received — disclosed Codex assistance in its body. The standard here is not which tools you used; it is whether you can defend the diff line by line when someone questions it, and whether the claims in the description match the code. A disclosure line like #150's is welcome and costs you nothing.

What the last hour demonstrates is the argument for review, not against tooling: a human read this carefully and found a counter bug that the tests did not, and separately the same person found that three assertions in test_no_chrome.py cannot fail (#155). Both would have been just as easy to miss in hand-written code.

One scheduling note: #153 also modifies moon_cli.py (exit codes, #32) and is currently conflicting with main. Whichever of the two lands first, the other will need a rebase. I will sequence them and neither of you needs to coordinate it.

Marked as a comment rather than changes-requested because it is still a draft. Fix the counter and mark it ready and I will take another pass.

[XEDAB]

@breezeFur — the shape of this is right. Keeping the CLI's own queue and scheduling and using the engine only as the progress owner is the version of #97 I was hoping for; the alternative, routing the whole run through Engine.start(), was tried on the same day and hits problems this approach avoids entirely.

@XEDAB's review comment is correct and it blocks. I verified it against the source rather than taking either of your words for it.

mark_extraction() bumps _url_done unconditionally:

def mark_extraction(self, success: bool):
    with self._lock:
        self._url_done += 1

Your two success call sites are not guarded, while the failure site is:

progress.mark_extraction(True)          # ← both success paths, unguarded
...
if not success and not is_re and not fatal_control.is_set():
    ...
    if progress.mark_extraction(False): # ← guarded by `not is_re`

and moon_cli.py:196 defines is_re = rec.stall_kills > 0, with the re-queue at moon_cli.py:167. So a stall-killed URL comes back through the worker, extracts successfully a second time, and increments _url_done again for the same URL. _url_total is len(urls), one per URL, so the counter is now measuring a different quantity from its denominator.

The consequence @XEDAB describes is real, at moon_engine.py:743:

elif url_done < url_tot:
    stage = "extracting"

Once the double-counting pushes url_done to url_tot, stage leaves extracting while re-extractions are still in flight, and your extracting {extract_done}/{extract_total} line can print a numerator past its denominator. On a run with stall kills — which is the run this project has most — that is visible.

Counting unique URLs is the right fix, as they suggested. Guarding the success calls with not is_re would also work and is smaller; either is fine, but say which you chose and why in the body.

On the [codex] prefix, since @XEDAB raised it: it is not a problem and no one needs to hide it. This repository's own commits carry Co-Authored-By: Claude, and #150 — the most careful pull request this project has received — disclosed Codex assistance in its body. The standard here is not which tools you used; it is whether you can defend the diff line by line when someone questions it, and whether the claims in the description match the code. A disclosure line like #150's is welcome and costs you nothing.

What the last hour demonstrates is the argument for review, not against tooling: a human read this carefully and found a counter bug that the tests did not, and separately the same person found that three assertions in test_no_chrome.py cannot fail (#155). Both would have been just as easy to miss in hand-written code.

One scheduling note: #153 also modifies moon_cli.py (exit codes, #32) and is currently conflicting with main. Whichever of the two lands first, the other will need a rebase. I will sequence them and neither of you needs to coordinate it.

Marked as a comment rather than changes-requested because it is still a draft. Fix the counter and mark it ready and I will take another pass.

About the [codex] prefix — I wasn't criticising AI-written code. without them like these I'd still be stuck on the Python book with the snake on the cover. I was just curious about the prefix. Sorry if it sounded like more than that.

[LeyckerS]

@XEDAB — no apology needed, and it did not sound like more than that. You asked a straightforward question about an unusual prefix and got a straightforward answer; the paragraph was for anyone else reading, not aimed at you.

For what it is worth, your last two days are the argument I was making. You read a pull request carefully enough to find a counter bug the tests missed, then found an assertion that four tests had been running for weeks without it ever being able to fail, then fixed it properly on the second pass after I gave you a wrong specification twice. That is reviewing, and it is the part no tool does for you.

#157 is merged. #160 is open off the back of it — the download_file stub is applied to moon_cli only, so the engine has been holding the real function all along and nothing noticed. It is yours if you want it; otherwise it is free and you owe this project nothing.

without them like these I'd still be stuck on the Python book with the snake on the cover

That book is how a lot of people here started, and plenty never got past it. You are reading concurrency code and finding real bugs in it. Do not sell that short.