Skip to content

fix(translate): reject short ok arrays and negative numeric error codes - #25

Open
devrishik wants to merge 2 commits into
masterfrom
fix/translate-envelope-validation
Open

devrishik wants to merge 2 commits into
masterfrom
fix/translate-envelope-validation

Conversation

@devrishik

Copy link
Copy Markdown

Follow-up on merged #23 (boss review findings 3+4). Findings 1/2/5 deferred as taste.

Finding #3

parse_translate_response accepted a per-string ok array shorter than sent. An all-true short array (e.g. ok:[true] for three texts) passed and the missing flags silently covered unsent fields.

Fix: require flags.len() == sent.len(); mismatch is a malformed envelope (bail!). Same-length all-true still passes.

Finding #4

Numeric negative error codes ("code": -1) were skipped: as_u64() is None on signed JSON numbers and as_str() is None on numbers.

Fix: parse as i64 (as_i64 first, then as_u64i64, then string). Negative codes are error envelopes. 0 and 200 stay success. Do not fold i64→u64 (try_from would drop negatives again).

Deferred (taste)

Tests

  • ok_array_shorter_than_sent_is_malformed
  • numeric_negative_error_code_is_a_hard_failure

cargo test --bin zola -- ok_array_shorter_than_sent numeric_negative_error_code ok_false ok_array_with_a_false mirrored_error_code_1003 ok_all_true → 7 passed.

Lock scope for ok-array length + negative numeric error codes.
Findings 1/2/5 deferred as taste.
Finding #3: require flags.len() == sent.len() so a short all-true ok
array cannot pass as a valid envelope.

Finding #4: parse code/status/error_code as i64 (then u64, then string)
so numeric negatives like "code": -1 are treated as error envelopes.
Do not fold i64 into u64 (drops signed codes).

Findings 1/2/5 deferred as taste.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 93fe2334-4290-44b7-a865-608826f80d25


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devrishik

Copy link
Copy Markdown
Author

Automated GLM review — human review still required.

Scope: src/cmd/translate.rs changes only (loops/ docs excluded). Both fixes verified correct; tests re-run locally (cargo test --bin zola -- … → 7 passed, 0 failed). No approval, no merge, no code changes from this review.

1. should-fix — src/cmd/translate.rs:265 — u64-range positive error codes are now silently dropped
v.as_u64().and_then(|n| i64::try_from(n).ok()) is dead for its stated purpose: any u64 that fits i64 was already consumed by as_i64() at :264, so the only values reaching this arm are > i64::MAX, where try_from fails → None → code silently treated as success. Net regression vs old code: {"code": 18446744073709551615} used to bail! via as_u64; now it passes. Same gap via the string arm at :266: "code": "18446744073709551615" parses as neither i64 (overflow) nor the old u64 inference → dropped. The comment ("u64 → i64 is only for values that did not fit as_i64") claims the opposite of what executes. Practically absurd codes, but this PR's whole point is "error codes must not be dropped." Suggestion: drop the middle arm; instead, when v.is_number() (or v.as_str() numeric) but all parse attempts yield None, bail!("translate endpoint: unparsable code in {key}"). That closes both gaps and deletes the dead arm.

2. nit — src/cmd/translate.rs:238 — longer ok arrays now also rejected; endpoint contract unverified
The length guard rejects flags.len() > sent.len() too. Symmetric with the pre-existing translations length check, so consistent — but body-only chunk requests send sent.len()==1, and if the nllb endpoint ever echoes a fixed 3-flag ok array regardless of texts count, every long-page body chunk hard-fails. Test at :912 only covers the shorter case. Suggestion: add ok_array_longer_than_sent_is_malformed, and confirm the endpoint mirrors texts length in ok (one curl against the live endpoint settles it).

3. nit — src/cmd/translate.rs:954 — string negative code path untested
:266 now also makes "code": "-1" a hard failure (string parse target changed u64→i64). That's new behavior with no test. Suggestion: mirror the numeric test with {"code":"-1","translations":["T"]}.

4. nit — src/cmd/translate.rs:247unwrap_or("?") fallback now unreachable
Harmless belt-and-suspenders after the :238 length guard; could drop .copied().unwrap_or("?") for a direct index. Leave if you prefer the guard-independent form.

Checked and clean: as_bool().unwrap() at the position closure is short-circuit-safe (unwrap only reached on Value::Boolean); per-chunk envelope validation stays consistent with 6000-char body chunking (each chunk gets its own sent/ok/translations triple); empty fields never sent; parse_translate_response signature untouched (deferred findings 1/2/5 respected); error messages carry both the code value and the offending key.

Findings: 0 blockers, 1 should-fix, 3 nits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant