refactor(plugin): migrate candidate normalization to TypeScript - #758
refactor(plugin): migrate candidate normalization to TypeScript#758mldangelo-oai wants to merge 9 commits into
Conversation
Generate the standalone helper during plugin packaging and retain Python for production. Add differential CLI, filesystem, Unicode, and property coverage, including cached-plugin upgrades and cyclic-link validation.
…ize-candidates-ts-ready
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a594de952
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const contents = readFileSync(path, "utf8"); | ||
| return contents === "" | ||
| ? 0 | ||
| : contents.split("\n").length - (contents.endsWith("\n") ? 1 : 0); |
There was a problem hiding this comment.
When a repository source file uses CR-only line endings, this counts every nonempty file as one line because it only splits on \n; the existing Python normalizer counts \r, \n, and \r\n via splitlines(). Consequently, valid candidate locations beginning on line 2 or later are rejected as out of range, so preserve the existing line-count behavior when validating locations.
AGENTS.md reference: AGENTS.md:L32-L33
Useful? React with 👍 / 👎.
mldangelo-oai
changed the title
|
@codex review Please review the current head, c786f7d. This update switches production callers to the Node helper, removes the Python implementation, and leaves plugin versions unchanged. It also fixes CR-only source-line counting and refreshes the reviewed diff-scan workflow fingerprint used by custom validation. |
mldangelo-oai
changed the title
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07cd80acbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!allowMissing || !isMissingFile(error)) { | ||
| throw new Error(`in-scope file row ${index + 1}: ${message(error)}`); | ||
| } |
There was a problem hiding this comment.
When --allow-missing-in-scope is used and a row such as linked/deleted.ts traverses a repository symlink to an outside directory where the final file is absent, realpathSync raises ENOENT and this branch silently drops the row before checking containment. The replaced implementation resolved missing paths non-strictly and rejected escapes, so a malformed or tampered diff inventory can now pass normalization despite containing an unsafe path; validate the canonical missing path or its existing parent before skipping it.
AGENTS.md reference: AGENTS.md:L30-L33
Useful? React with 👍 / 👎.
| const { values } = parseArgs({ | ||
| args: argv, | ||
| options: { | ||
| input: { type: "string", multiple: true }, |
There was a problem hiding this comment.
The replaced command declared --input with nargs="+", so callers can pass --input first.jsonl second.jsonl; Node's parseArgs with multiple: true consumes only one value per occurrence and rejects second.jsonl as an unexpected positional argument. This changes an accepted public invocation during a runtime-language refactor, so retain the one-flag/multiple-files form, optionally in addition to repeated --input flags.
AGENTS.md reference: AGENTS.md:L39-L49
Useful? React with 👍 / 👎.
| function readLines(path: string): string[] { | ||
| return new TextDecoder("utf-8", { fatal: true }) | ||
| .decode(readFileSync(path)) | ||
| .split(/\r?\n/u); |
There was a problem hiding this comment.
On POSIX, an LF-delimited inventory can legitimately contain a filename ending in a literal carriage return, but this split always interprets the resulting \r\n bytes as a line terminator and removes the carriage return. The previous reader deliberately disambiguated CRLF from literal-CR filenames, so candidates for such files are now rejected as out of scope—or a different sibling path can be treated as in scope when both names exist. Preserve the existing path-name behavior while recognizing CRLF input.
AGENTS.md reference: AGENTS.md:L28-L31
Useful? React with 👍 / 👎.
2 participants
Summary
Migrate candidate normalization from Python to TypeScript. Discovery and diff scans now run the Node helper, and the Python implementation is removed.
Changes
normalize_candidates.mjsthrough the existing plugin build.Testing
git diff --checkpassed; runtime code is unchanged since the test runs above.Risk and rollout
The helper invocation is now
node normalize_candidates.mjs. Repeat--input FILEfor multiple inputs, use full option names, and let the shell expand home paths. Candidate IDs may differ from Python output; stored scan artifacts are not migrated automatically.No top-level CLI or version changes. The plugin stays at
0.1.79; cached installations receive the migration with the next versioned release. Other helpers still require Python. Linux, Windows, and Node 26 were not tested locally.Public disclosure review