Repository navigation
Harden agent arg parsing to avoid accidental key matches and malformed token handling - #24
Open
ANANDHA007 wants to merge 1 commit into
Open
ANANDHA007 wants to merge 1 commit into
ANANDHA007 wants to merge 1 commit into
Conversation
…n logs and Unit test is added
Reviewer's GuideRefactors agent argument parsing into a strict key=value map-based parser with explicit validation methods, adds handling for malformed/unknown tokens and duplicate keys, and updates unit tests to cover the new behavior and error conditions. Flow diagram for strict agent argument parsing and validationflowchart TD
A[agentArgs string] --> B[parseArgs]
B --> C[Split by comma into tokens]
C --> D[For each token]
D --> E{Empty or null token?}
E -- Yes --> D
E -- No --> F[Split token by = into parts]
F --> G{parts length == 2?}
G -- No --> H[warn Ignoring malformed token]
H --> D
G -- Yes --> I[Trim key and value]
I --> J{key empty?}
J -- Yes --> K[warn Ignoring token with empty key]
K --> D
J -- No --> L{key in SUPPORTED_KEYS?}
L -- No --> M[warn Ignoring unknown key]
M --> D
L -- Yes --> N{key already in parsed map?}
N -- Yes --> O[throw IllegalArgumentException Duplicate argument]
N -- No --> P[parsed.put key,value]
P --> D
D -->|All tokens processed| Q[parsed Map<String,String>]
Q --> R[validateConfigFile]
Q --> S[validateAgentLogFileDir]
Q --> T[validateAgentLogLevel]
Q --> U[validateSmtpProperties]
Q --> V[validateAgentJarPath]
R --> W[AgentLaunchOptions]
S --> W
T --> W
U --> W
V --> W
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new validation methods treat empty-string values as
null(e.g.,configFile=oragentLogLevel=), which is a behavioral change from the previous implementation that rejected missing values; consider either preserving the old strictness or explicitly documenting/handling empty values as an error. validateAgentLogLevelnow accepts case-insensitive values but returns the original string, which may cause inconsistencies downstream (e.g., if consumers expectLogLevel-style upper case); consider normalizing to a canonical form (such aslevel.name()) before returning.- The
warnhelper currently usesSystem.errdirectly; if this parser is used in long-running or production contexts, consider integrating with the existing logging framework or injecting a logger to keep logging behavior consistent and configurable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new validation methods treat empty-string values as `null` (e.g., `configFile=` or `agentLogLevel=`), which is a behavioral change from the previous implementation that rejected missing values; consider either preserving the old strictness or explicitly documenting/handling empty values as an error.
- `validateAgentLogLevel` now accepts case-insensitive values but returns the original string, which may cause inconsistencies downstream (e.g., if consumers expect `LogLevel`-style upper case); consider normalizing to a canonical form (such as `level.name()`) before returning.
- The `warn` helper currently uses `System.err` directly; if this parser is used in long-running or production contexts, consider integrating with the existing logging framework or injecting a logger to keep logging behavior consistent and configurable.
## Individual Comments
### Comment 1
<location path="src/test/java/com/asm/mja/args/AgentArgsParserTest.java" line_range="21-28" />
<code_context>
*/
public class AgentArgsParserTest {
@Test
void parseReturnsNullFieldsWhenAgentArgsMissing() {
+
</code_context>
<issue_to_address>
**issue (testing):** Test name suggests duplicate-key behavior, but the body only exercises `validateAgentLogFileDir` and not duplicate argument parsing
This asserts the `validateAgentLogFileDir` error for a non-existent directory, but the name (`parseThrowsForDuplicateKeys`) implies we’re testing `parse` rejecting duplicate keys. That both misleads and leaves `parseArgs`’s duplicate-key behavior untested. Please either rename the test to match its current behavior or, preferably, change it to pass a duplicate key (e.g. `agentLogLevel=INFO,agentLogLevel=DEBUG`) to `AgentArgsParser.parse(...)` and assert the expected `IllegalArgumentException` and message about duplicate arguments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
21
to
28
| @Test | ||
| void parseReturnsNullFieldsWhenAgentArgsMissing() { | ||
|
|
||
| AgentLaunchOptions options = AgentArgsParser.parse(null); | ||
|
|
||
| assertNull(options.getConfigFile()); | ||
| assertNull(options.getAgentLogFileDir()); | ||
| assertNull(options.getAgentLogLevel()); |
There was a problem hiding this comment.
issue (testing): Test name suggests duplicate-key behavior, but the body only exercises validateAgentLogFileDir and not duplicate argument parsing
This asserts the validateAgentLogFileDir error for a non-existent directory, but the name (parseThrowsForDuplicateKeys) implies we’re testing parse rejecting duplicate keys. That both misleads and leaves parseArgs’s duplicate-key behavior untested. Please either rename the test to match its current behavior or, preferably, change it to pass a duplicate key (e.g. agentLogLevel=INFO,agentLogLevel=DEBUG) to AgentArgsParser.parse(...) and assert the expected IllegalArgumentException and message about duplicate arguments.
Owner
|
Follow-up with clean formatting (same requested changes): Request changes before merge
After these updates and green CI, this should be ready to merge. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refac:
Summary by Sourcery
Harden agent argument parsing to be strict and resilient to malformed input while centralizing validation logic.
New Features:
Bug Fixes:
Enhancements:
Tests: