📟 Operator: Fix Exit Code Lies in pebble next --json - #287
Conversation
Moves the emptiness check for 'next' command above the JSON formatting block,
ensuring that `pebble next --json` correctly fails with an exit code of 1
and an error message on stderr instead of silently succeeding with `{"tasks":[]}`.
Added operator standard note in `.jules/operator.md` to document the convention.
Co-authored-by: matta <37314+matta@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
|
Summary of ChangesHello, 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 addresses an inconsistency in the 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
|
![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 correctly fixes an issue where pebble next --json would return a success exit code (0) even when no tasks were found, which is problematic for scripting. The fix involves moving the check for empty tasks before the JSON serialization logic. The associated tests have been updated to cover this new failure case, and a new entry has been added to the operator's manual.
My review includes two suggestions for improvement. First, in the integration test, I recommend strengthening the assertion to check for the specific error message on stderr, aligning with the principle of providing informative test failures. Second, in the operator's manual, I've noted that the new entry is very similar to an existing one and suggest consolidating them to avoid redundancy and improve clarity.
| ## 2025-02-24 - Exit Code Lies in Search and Retrieval Commands | ||
| **Friction:** `pebble next --json` returned exit code 0 and an empty JSON array `{"tasks":[]}` when no tasks were found, whereas the human-readable output correctly returned a non-zero exit code. | ||
| **Insight:** Single-item search or retrieval commands (like `next` or `show`) must consistently return non-zero exit codes when the requested item is missing, regardless of the output format. Returning "null" or an empty array with exit code 0 breaks CI/CD and shell script automation that relies on exit codes for control flow. | ||
| **Standard:** Search/retrieval commands must return exit code 1 when the requested item is missing. Always check for empty results and return `Err(NotFoundError(...).into())` *before* attempting to serialize or format the output for `--json`. |
There was a problem hiding this comment.
This new entry is very similar to the existing one on lines 7-9. While this entry is more specific to the --json flag and more accurate about the previous state, having two very similar standards about exit codes for 'not found' scenarios could be confusing.
Consider consolidating them into a single, comprehensive standard. For example, you could update the existing entry to incorporate the details about --json output, or replace both with a new, unified entry. This would improve clarity and maintainability of these operational standards.
| .expect("pebble command should execute successfully"); | ||
|
|
||
| assert!(!output.status.success()); | ||
| assert!(!output.stderr.is_empty()); |
There was a problem hiding this comment.
The assertion on stderr is a bit weak. It only checks that stderr is not empty, but it doesn't verify the error message content. This could lead to the test passing even if the command fails for an unexpected reason. It's better to assert that stderr contains the specific error message for 'no ready tasks'. This aligns with the principle of providing informative error messages in tests, similar to using expect() over unwrap().
| assert!(!output.stderr.is_empty()); | |
| assert!(String::from_utf8_lossy(&output.stderr).contains("No ready tasks found.")); |
References
- In tests, including doctests, use
expect()with a descriptive message instead ofunwrap()to provide more informative error messages on failure. This principle extends to making assertions more specific to ensure tests fail informatively.
1 participant
😫 Friction:
pebble next --jsonwas returning exit code 0 when there were no ready tasks, unlike the human-readablepebble nextformat which failed with an exit code of 1. Returning success on a query that inherently failed to find an item is an "Exit Code Lie" that breaks automation scripts (likewhile pebble next --json; do ... done).🤖 Fix: Modified
run_nextincrates/pebble/src/commands.rsto move the.is_empty()check above theif ctx.jsonblock.📉 Before:
pebble next --json->{"tasks":[]}(exit code: 0)📈 After:
pebble next --json->Task 'fake' not foundon stderr (exit code: 1)PR created automatically by Jules for task 10000198630323544085 started by @matta