feat: disambiguate input format with explicit --format option #45
Conversation
Change --format from OutputFormat (default hex) to Option<OutputFormat>. When explicitly specified, decode_input uses that format directly without fallback, preventing misdetection of base64 data as hex. When omitted, auto-detection (PEM -> hex -> base64) is preserved for backward compatibility. Output defaults to hex when --format is not specified. Add unit tests for decode_input with explicit/auto-detect modes and integration tests for explicit format roundtrip, format mismatch error messages, and default hex output verification.
Summary of ChangesHello @kiyoaki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLI's robustness and user control over data encoding and decoding. By introducing an explicit Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Updates the CLI to treat --format as an explicit input encoding override (when provided), while preserving auto-detection when omitted, to prevent ambiguous hex/base64 misdetection.
Changes:
- Switched
--formatfromOutputFormat(defaulthex) toOption<OutputFormat>across commands and decoding logic. - Updated
decode_inputto honor explicit format without fallback, preserving auto-detect only when--formatis omitted. - Added unit + integration tests covering explicit format behavior, mismatch errors, and default hex output.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| kylix-cli/src/io.rs | Implements explicit-vs-auto decode behavior; adds decode helpers and unit tests. |
| kylix-cli/src/cli.rs | Makes --format optional and updates help text across commands. |
| kylix-cli/src/commands/keygen.rs | Defaults output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/encaps.rs | Defaults ciphertext/shared-secret output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/decaps.rs | Defaults shared-secret output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/sign.rs | Defaults signature output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/verify.rs | Updates command signature to accept optional format. |
| kylix-cli/tests/cli_integration.rs | Adds integration tests for explicit format roundtrip, mismatch errors, and default hex output. |
| PLANS.md | Marks the M-3 plan item as implemented with the new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, introducing an explicit --format option to disambiguate input formats and prevent auto-detection errors. The change from OutputFormat to Option<OutputFormat> is applied consistently across all relevant commands. The addition of comprehensive unit and integration tests significantly improves the robustness of the input decoding logic. I have a couple of suggestions to further improve the new PEM decoding logic and Rust code style.
- Validate -----END footer in decode_pem (Copilot + Gemini) - Use generic error message listing all formats: hex|base64|pem (Copilot) - Refactor None branch to if/else if/else expression (Gemini) - Use &Path instead of to_str().unwrap() in new integration tests (Copilot) - Use input-only help text for verify's --format arg (Copilot)
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
- Extract OutputFormat::DEFAULT constant to avoid repeating Hex default across 4 command files (keygen, encaps, decaps, sign) - Add [L-6] PEM Label Validation to PLANS.md for future BEGIN/END label mismatch detection in decode_pem
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
- Handle CRLF line endings in decode_pem by normalizing \r (Copilot) - Replace OutputFormat::DEFAULT with #[derive(Default)] + #[default] and use unwrap_or_default() for idiomatic Rust (Copilot) - Fix test_default_output_is_hex: add empty check and use floor_char_boundary for safe string slicing (Copilot)
|
/gemini review |
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by disambiguating input formats through an explicit --format option. The changes correctly transition the format argument to Option<OutputFormat> across all relevant commands, allowing users to specify the encoding or rely on robust auto-detection. The decode_input function has been refactored to prioritize explicit format choices and provides helpful error messages when a format mismatch occurs. The addition of a Pem variant to OutputFormat and setting Hex as the default for output when unspecified are well-implemented. The comprehensive suite of unit and integration tests thoroughly validates the new behavior, covering explicit format decoding, fallback prevention, and auto-detection scenarios. Overall, this is a well-executed feature that enhances usability and clarity for input handling.
- Remove line continuation in error messages to avoid embedded indentation spaces in output (Copilot) - Simplify test prefix slice to trimmed.get(..40).unwrap_or(trimmed) instead of floor_char_boundary (Copilot)
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if lines.len() < 3 | ||
| || !lines[0].starts_with("-----BEGIN") | ||
| || !lines.last().unwrap().starts_with("-----END") | ||
| { | ||
| bail!("Invalid PEM format: expected -----BEGIN header, body lines, and -----END footer"); | ||
| } |
There was a problem hiding this comment.
The PEM header/footer validation is permissive: it only checks starts_with and does not ensure the BEGIN/END labels match or that the footer line is well-formed. This can accept malformed PEM like -----END X-----garbage or mismatched labels. Consider parsing the label from the BEGIN line and requiring the END line to exactly match -----END {label}----- (and rejecting extra trailing characters/lines).
| if lines.len() < 3 | |
| || !lines[0].starts_with("-----BEGIN") | |
| || !lines.last().unwrap().starts_with("-----END") | |
| { | |
| bail!("Invalid PEM format: expected -----BEGIN header, body lines, and -----END footer"); | |
| } | |
| if lines.len() < 3 { | |
| bail!("Invalid PEM format: expected -----BEGIN header, body lines, and -----END footer"); | |
| } | |
| // Validate and parse the BEGIN line: -----BEGIN <LABEL>----- | |
| let begin_line = lines[0]; | |
| const BEGIN_PREFIX: &str = "-----BEGIN "; | |
| const PEM_SUFFIX: &str = "-----"; | |
| if !begin_line.starts_with(BEGIN_PREFIX) || !begin_line.ends_with(PEM_SUFFIX) { | |
| bail!("Invalid PEM header: expected line of the form '-----BEGIN <LABEL>-----'"); | |
| } | |
| let label = &begin_line[BEGIN_PREFIX.len()..begin_line.len() - PEM_SUFFIX.len()]; | |
| if label.is_empty() { | |
| bail!("Invalid PEM header: empty label"); | |
| } | |
| // Validate END line exactly matches the parsed label: -----END <LABEL>----- | |
| let end_line = lines.last().unwrap(); | |
| let expected_end = format!("-----END {}-----", label); | |
| if end_line != &expected_end { | |
| bail!("Invalid PEM footer: expected '{}'", expected_end); | |
| } | |
| // Join all body lines between BEGIN and END as base64 content |
| Some(OutputFormat::Hex) => hex::decode(data).context( | ||
| "Failed to decode as hex. If the input uses a different encoding, remove --format or set it to the encoding used by the input: hex|base64|pem.", | ||
| ), | ||
| Some(OutputFormat::Base64) => BASE64.decode(data).context( | ||
| "Failed to decode as base64. If the input uses a different encoding, remove --format or set it to the encoding used by the input: hex|base64|pem.", | ||
| ), |
There was a problem hiding this comment.
The hex/base64 branches duplicate a long, mostly identical guidance string. Consider extracting a small helper to format the message (e.g., based on the requested OutputFormat) to keep messaging consistent and make future edits less error-prone.
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, successfully disambiguating input formats by making the --format flag optional. When provided, it strictly enforces the format, and when omitted, it maintains backward compatibility with auto-detection. The changes are consistently applied across all relevant commands. The addition of comprehensive unit and integration tests is excellent and covers the new logic well, including edge cases like hex-looking base64 data. I have a couple of minor suggestions for improving code clarity and error handling in io.rs.
| let normalized; | ||
| let data = if data.contains('\r') { | ||
| normalized = data.replace('\r', ""); | ||
| &normalized | ||
| } else { | ||
| data | ||
| }; |
There was a problem hiding this comment.
For better readability and to use a more idiomatic Rust pattern for conditional allocation, you could use std::borrow::Cow (Clone-on-Write) to handle the optional string normalization. This avoids the slightly complex lifetime management with the normalized variable and makes the intent clearer.
let data: std::borrow::Cow<str> = if data.contains('\r') {
std::borrow::Cow::Owned(data.replace('\r', ""))
} else {
std::borrow::Cow::Borrowed(data)
};| } else if is_hex(data) && data.len() % 2 == 0 { | ||
| hex::decode(data).context("Failed to decode hex") | ||
| } else { | ||
| BASE64.decode(data).context("Failed to decode base64") |
There was a problem hiding this comment.
The error message here could be more helpful to the user. Since this is the last step in auto-detection, if it fails, it's likely the input is in an unsupported format or is malformed. A more descriptive error could guide the user to specify the format explicitly with --format if they know it.
BASE64.decode(data).context("Failed to decode as base64. Auto-detection fell back to base64, but decoding failed. If you know the input format, try specifying it with --format.")1 participant
Summary
--formatfromOutputFormat(defaulthex) toOption<OutputFormat>across all 5 commands--formatis explicitly specified,decode_inputuses that format directly without fallback, preventing misdetection (e.g., base64 data that looks like valid hex)--formatis omitted, auto-detection (PEM -> hex -> base64) is preserved for backward compatibility--formatis not specifieddecode_inputcovering explicit format, fallback prevention, and auto-detect modesImplements [M-3] from PLANS.md.
Test plan
cargo fmt --all -- --checkpassescargo clippy --workspace --all-features -- -D warningspassescargo clippy --workspace --no-default-features -- -D warningspassescargo test --workspace --all-featurespasses (33 tests, 0 failures)