ClassifySequentiallyOptions.onProgress is typed as returning void, but callers can still pass an async function and the returned Promise is currently not awaited/handled here, which can lead to unhandled rejections or out-of-order progress reporting. Consider either enforcing a sync callback or explicitly supporting Promise<void> and awaiting it.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

git clean -fdx -e .analyzer-output hard-codes the preserved directory name, but outputDir is configurable via OUTPUT_DIR, so a non-default output directory would be deleted during resetWorkingTree() (undoing the new partial persistence). Consider ensuring the exclusion matches the configured outputDir.

Severity: high

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

onProgress is declared async here, but classifyCommitsSequentially does not await the callback. That makes it easy to accidentally add awaited work later (e.g., file writes) that will run concurrently/out of order and can hide errors. Either remove async on this callback (since it’s just logging) or change onProgress to return Promise<void> and await it in the classifier.

resetWorkingTree hard-codes -e .analyzer-output, but the pipeline’s outputDir is configurable via env (OUTPUT_DIR). If a non-default outputDir is used, git clean -fdx can delete the directory and wipe the artifacts you just started persisting. Consider passing the configured outputDir into resetWorkingTree (or accepting an array of excludes) and excluding that path instead of a constant.

Both git reset and git clean are executed with .nothrow(), so a failure to reset/clean (e.g., not a git repo, permissions issues) will be silently ignored and the later checkout/cherry-pick errors will be harder to diagnose. It would be safer to let these commands throw, or explicitly check exitCode and throw a targeted error when non-zero.

The comment says -fdx removes ignored files including .analyzer-output, but the command excludes .analyzer-output via -e .analyzer-output. Consider updating the comment to reflect that the analyzer output directory is intentionally preserved while other ignored/untracked files (e.g., node_modules) are removed.

🔴 Hardcoded git-clean exclusion path doesn't match configurable outputDir, deleting progressive artifacts

resetWorkingTree() at script/upstream-analyzer/git-inspector.ts:59 hardcodes -e .analyzer-output, but the output directory is configurable via the OUTPUT_DIR env var (script/upstream-analyzer/cli-config.ts:50). When OUTPUT_DIR is set to anything other than .analyzer-output, the git clean -fdx in resetWorkingTree() will delete all artifacts written before it runs. In pipeline.ts, four artifact files are written before resetWorkingTree() is called (lines 152, 159, 160, 163), and then resetWorkingTree() is invoked inside runBatchBuilding (line 104). This silently destroys classifications.initial.json, verifications.json, classifications.json, and synthesis.json — defeating the stated purpose of writing artifacts progressively so that "partial progress survives any downstream failure."

The `resetWorkingTree` function hardcodes the git-clean exclusion path as `.analyzer-output`, but the actual output directory is configurable via `AnalyzerConfig.outputDir` (set from the `OUTPUT_DIR` env var). When the two differ, `git clean -fdx` deletes all previously-written artifacts.

To fix this, `resetWorkingTree` should accept the output directory as a parameter and use it in the `-e` flag. The call site in `pipeline.ts:runBatchBuilding` (line 104) should pass `config.outputDir` (or receive it as a parameter). For example:

- In git-inspector.ts, change `resetWorkingTree()` to `resetWorkingTree(excludePaths: string[])` and build the `-e` flags dynamically.
- In pipeline.ts `runBatchBuilding`, pass `config.outputDir` so it gets excluded from the clean.

Was this helpful? React with 👍 or 👎 to provide feedback.

📝 Info: Async onProgress callback's returned Promise is silently dropped

In pipeline.ts:57, the onProgress callback is declared async, but the type in commit-classifier.ts:97 specifies => void (not => Promise<void>). The call site at commit-classifier.ts:111 does not await the result. This is not a bug today because the callback body (logProgress) is synchronous — the async keyword just wraps a resolved promise that's immediately GC'd. However, if someone later adds real async work (e.g., writing progress to a file) inside this callback, the returned promise would be silently dropped, potentially causing unhandled rejections or lost operations. Removing the unnecessary async keyword or changing the type to Promise<void> and await-ing the call would be defensive.

Was this helpful? React with 👍 or 👎 to provide feedback.

📝 Info: RateLimiter uses simple interval spacing rather than sliding window

The RateLimiter in throttle.ts enforces a minimum interval between requests (e.g., 5000ms for 12 req/min) rather than tracking a sliding window of timestamps. This is a simpler and more conservative approach — it can never exceed the limit but may underutilize capacity after pauses (e.g., if a request takes 10 seconds, the next request still waits the full interval from completion). For this sequential-only use case this is perfectly fine, but worth noting if someone considers reusing this class for concurrent workloads.

Was this helpful? React with 👍 or 👎 to provide feedback.

📝 Info: resetWorkingTree removes node_modules with -fdx, subsequent batch building still works

The git clean -fdx at git-inspector.ts:59 removes node_modules (and all other untracked/ignored files). This is intentional per the comment — the CI clone is ephemeral. However, buildAllBatches (called right after) only uses git operations (createBranchFromTag, cherryPickCommit) and doesn't need node_modules, so this is fine. But if the batch building phase were ever extended to require bun commands or imports from node_modules, this would break.

Was this helpful? React with 👍 or 👎 to provide feedback.

Guard destructive reset to CI-only execution

resetWorkingTree() now hard-resets and cleans the repository on every analyzer run, but there is no runtime guard that limits this behavior to ephemeral CI clones. Since script/upstream-analyzer/run.ts is a normal executable entrypoint, invoking it from a local checkout with uncommitted work will irreversibly discard tracked edits, which is a high-impact regression in safety.

Useful? React with 👍 / 👎.

Exclude configured output directory from git clean

The clean command hard-codes -e .analyzer-output even though OUTPUT_DIR is configurable in loadConfigFromEnv(). When OUTPUT_DIR points to another in-repo path, the new pre-batch git clean -fdx deletes pass-1/2/3 artifacts written earlier in runPipeline(), defeating partial-progress persistence for that configuration.

Useful? React with 👍 / 👎.