[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.

[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.

[LeyckerS]

@breezeFur — the counter fix is right, and I checked it rather than reading it.

def mark_extraction(self, url: str, success: bool):
    """Record one URL's final extraction result at most once."""
    with self._lock:
        if url in self._progress_extracted:
            return self._dl_done >= self._dl_total
        self._progress_extracted.add(url)
        self._url_done += 1

Keying on the URL and short-circuiting on a repeat is the "count unique URLs" fix @XEDAB suggested, and it is the right one — a re-extracted link now increments once no matter how many times it comes back through the worker. _progress_extracted is initialised in __init__ and cleared in start(), so it does not survive into the next run. The whole thing is inside the existing lock. pytest tests/ -q on your branch: 53 passed.

It needs a rebase before I can merge it, and that is recent — GitHub's "no conflicts" badge is stale. I trial-merged your branch into current main locally:

CONFLICT in moon_engine.py, line 141

#167 merged an hour ago and deleted _LOG_MAX_LINES = 2000 from exactly the region your diff touches. Your branch also predates four other merges — #161 added pytest.ini, #162 fixed the conftest stub, #165 changed the workflow paths, #158 split the ruff job (which is why your check list has no ruff entry while everyone else's does).

The conflict itself is trivial: keep your side and drop the deleted constant. Nothing about your logic is affected.

Once it is rebased I will re-run the suite against the merge result rather than the branch, and merge. Thank you for the fix and for the wait — the delay after you marked it ready is on me, not you.

[breezeFur]

Updated this branch with current main via a regular merge, without rewriting the reviewed history. The conflict in moon_engine.py was resolved by keeping the external-progress methods and retaining upstream's removal of _LOG_MAX_LINES.

Validation:

• python -m pytest tests/ -q: 66 passed
• python -m ruff check .: passed

[LeyckerS]

@breezeFur — thanks for the update; the merge is exactly what I asked for. I checked the conflict resolution (_LOG_MAX_LINES gone, external-progress methods kept), the suite is green at 66, and every ask from both earlier rounds is addressed.

Before the last step, though, I owe this thread a correction of my own. The unique-URL guard that @XEDAB proposed and I verified on 17 August fixes the stall-kill double count — that part stands; I re-traced it against cb95974 and a stall-killed, re-extracted URL counts exactly once. But neither of us exercised the other case the guard touches: the same URL twice in the input file. main() does not dedupe links.txt (moon_cli.py:376), and Telemetry.reg deliberately supports duplicates — each call returns a distinct FileRecord (moon_download.py:238). With the guard keyed on URL, the second occurrence becomes invisible: on its terminal extraction failure the early return in mark_extraction skips the _dl_done increment, so dl_done can never reach dl_total, all_done never fires, and run() blocks forever in the final gather. I drove your branch's engine through the CLI's exact call sequence to confirm: dl_done ends 1/2 with no event left that can move it. Repro: a links.txt with the same unsupported-host URL on two lines — exit code 3 in seconds on main, an indefinite hang on this branch. There is also a cosmetic sibling: duplicates that all succeed complete fine, but the stage line reads extracting N-1/N for the whole run.

The fix is small and either shape is fine by me:

• dedupe urls at intake in main()/run() (order-preserving), or
• key the once-only guard on the FileRecord instead of the URL — the stall-kill re-queue re-puts the same record (moon_cli.py:167), so per-record keying preserves your fix exactly while restoring per-entry counting.

A test that feeds a duplicated URL through run() would nail it shut — no test on either branch does that today.

Second correction, smaller: my 17 August comment said _progress_extracted is "initialised in __init__ and cleared in start()". It is actually cleared in begin_external_progress(); start() never touches it. Harmless in practice — the CLI builds a fresh Engine per run — but the record should be straight.

One thing to state rather than ask: with elapsed_s excluded from the progress key, a fully stalled download prints nothing, so a frozen terminal is indistinguishable from suppression. Your body and the new docs describe this, so I am taking it as deliberate and I am fine with it — say if that was not the intent.

Fix the duplicate handling and this merges; everything else held up under a full re-check.