fix: declare --fields option in job:get command#29
Merged
Conversation
The --fields option was used in execute() but never registered in configure(), causing a runtime error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optional ChangesJob:Get field filtering
Development tooling (dev dependencies)
Debian packaging: icon and install mapping
Minor formatting / non-functional edits
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Command/Job/GetCommand.php (5)
57-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winField names parsed from
--fieldsare not trimmed, so--fields name, statussilently fails to matchstatus.
explode(',', $fields)preserves surrounding whitespace. Users naturally write--fields name, statusand expect both fields.🛠️ Proposed fix
- $fieldsArray = explode(',', $fields); + $fieldsArray = array_map('trim', explode(',', $fields));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Job/GetCommand.php` around lines 57 - 58, The field parsing uses explode(',', $fields) which leaves surrounding whitespace so entries like " status" won't match keys; update the parsing before using $fieldsArray by trimming each element (e.g. replace the explode call with an array_map('trim', explode(',', $fields)) or equivalent) so $fieldsArray contains trimmed field names, then keep the existing array_filter($job->getData(), ..., \ARRAY_FILTER_USE_KEY) logic unchanged.
24-36: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing docblocks for the class and both methods.
GetCommand,configure(), andexecute()all lack PHPDoc blocks.🛠️ Proposed fix
+/** + * Retrieves a single MultiFlexi job by its ID and outputs its data. + */ class GetCommand extends MultiFlexiCommand { protected static $defaultName = 'job:get'; + /** + * Configures the command options: id, fields, and format. + */ protected function configure(): void+ /** + * Executes the job:get command. + * + * `@param` InputInterface $input The console input. + * `@param` OutputInterface $output The console output. + * + * `@return` int Command exit code. + */ protected function execute(InputInterface $input, OutputInterface $output): intAs per coding guidelines, "Include a docblock for every function and class, describing its purpose, parameters, and return types."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Job/GetCommand.php` around lines 24 - 36, Add missing PHPDoc blocks for the GetCommand class and its configure() and execute() methods: add a class-level docblock above class GetCommand summarizing its purpose (CLI command to fetch a job by ID), and add method-level docblocks for configure() describing the configuration steps and for execute() describing parameters (InputInterface $input, OutputInterface $output), return type (int) and possible thrown exceptions; ensure descriptions follow project style and include `@param` and `@return/`@throws tags where applicable and reference the existing method names configure and execute in their respective docblocks.
53-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo existence check after loading the job — silently returns empty data with
SUCCESSfor an unknown ID.
new Job((int) $id)returns an unpopulated object when the ID doesn't exist. The command exitsSUCCESSwith an empty payload, giving the caller no indication that the job was not found.🛠️ Proposed fix
$job = new Job((int) $id); + + if (empty($job->getData())) { + if ($format === 'json') { + $output->writeln(json_encode(['status' => 'error', 'message' => "Job {$id} not found"], \JSON_PRETTY_PRINT)); + } else { + $output->writeln("<error>Job {$id} not found</error>"); + } + + return self::FAILURE; + } + $fields = $input->getOption('fields');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Job/GetCommand.php` around lines 53 - 61, After creating the Job with new Job((int) $id) in GetCommand, add an existence check on the loaded object (e.g. inspect $job->getId() or that $job->getData() is non-empty) and if the job was not found, write a clear error message to the console/output and return a non-zero status (Command::FAILURE) instead of proceeding to output empty data; keep the existing fields filtering logic but only run it when the existence check passes.
67-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winText output will raise a warning (or produce
"Array") when a job field value is non-scalar.
$vfrom$job->getData()can be an array or object. String interpolation"{$k}: {$v}"triggers anArray to string conversionnotice in PHP 8 and renders the literal string"Array". Serialize complex values before printing.🛠️ Proposed fix
- foreach ($data as $k => $v) { - $output->writeln("{$k}: {$v}"); - } + foreach ($data as $k => $v) { + $scalar = \is_array($v) || \is_object($v) + ? json_encode($v, \JSON_UNESCAPED_UNICODE) + : (string) $v; + $output->writeln("{$k}: {$scalar}"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Job/GetCommand.php` around lines 67 - 69, Non-scalar job field values cause an "Array to string conversion" warning when printing "{$k}: {$v}" from $job->getData(); update the loop in GetCommand (the foreach over $data and $output->writeln call) to serialize non-scalar values before outputting: for each $k/$v from $job->getData() check is_scalar/is_null and print directly, otherwise convert $v to a readable string (e.g. json_encode or var_export) and then call $output->writeln("{$k}: " . $serializedValue) so arrays/objects are rendered safely.
24-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPHPUnit test file must be created or updated for the Job\GetCommand class.
The class was modified but no corresponding PHPUnit test file exists. As per coding guidelines, "When creating or updating a class, always create or update its PHPUnit test file." Create
tests/src/Command/Job/GetCommandTest.phpwith appropriate unit tests for this command class.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Job/GetCommand.php` around lines 24 - 74, Add a new PHPUnit test file tests/src/Command/Job/GetCommandTest.php that covers GetCommand behavior: use Symfony\Component\Console\Tester\CommandTester to execute the GetCommand and assert outputs for (1) missing --id in text mode (expect failure exit code and "<error>Missing --id</error>"), (2) missing --id in json mode (expect JSON error with status "error" and message "Missing --id"), and (3) successful retrieval cases — mock or stub the Job data source so GetCommand (class GetCommand) returns a predictable array from Job::getData and assert that --fields filters keys correctly and that --format=json returns pretty JSON while text format prints "key: value" lines; ensure tests instantiate GetCommand, pass options 'format', 'id', and 'fields' to CommandTester, and assert exit codes SUCCESS/FAILURE and exact output content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Command/Job/GetCommand.php`:
- Around line 32-35: Wrap all user-facing literal strings in GetCommand.php with
the i18n helper `_()`: change the description passed to setDescription('Get a
job by ID') to setDescription(_('Get a job by ID')), and wrap each addOption
label/description values (the format option description 'Output format: text or
json', the id option name/description 'Job ID', and the fields option
description 'Comma-separated list of fields to display') using `_()`. Also
locate the two "Missing --id" messages referenced in the execute/validation
logic and wrap those literal messages with `_()` as well so all displayed
strings go through the i18n layer (target symbols: setDescription, addOption
calls, and the "Missing --id" error messages).
---
Outside diff comments:
In `@src/Command/Job/GetCommand.php`:
- Around line 57-58: The field parsing uses explode(',', $fields) which leaves
surrounding whitespace so entries like " status" won't match keys; update the
parsing before using $fieldsArray by trimming each element (e.g. replace the
explode call with an array_map('trim', explode(',', $fields)) or equivalent) so
$fieldsArray contains trimmed field names, then keep the existing
array_filter($job->getData(), ..., \ARRAY_FILTER_USE_KEY) logic unchanged.
- Around line 24-36: Add missing PHPDoc blocks for the GetCommand class and its
configure() and execute() methods: add a class-level docblock above class
GetCommand summarizing its purpose (CLI command to fetch a job by ID), and add
method-level docblocks for configure() describing the configuration steps and
for execute() describing parameters (InputInterface $input, OutputInterface
$output), return type (int) and possible thrown exceptions; ensure descriptions
follow project style and include `@param` and `@return/`@throws tags where
applicable and reference the existing method names configure and execute in
their respective docblocks.
- Around line 53-61: After creating the Job with new Job((int) $id) in
GetCommand, add an existence check on the loaded object (e.g. inspect
$job->getId() or that $job->getData() is non-empty) and if the job was not
found, write a clear error message to the console/output and return a non-zero
status (Command::FAILURE) instead of proceeding to output empty data; keep the
existing fields filtering logic but only run it when the existence check passes.
- Around line 67-69: Non-scalar job field values cause an "Array to string
conversion" warning when printing "{$k}: {$v}" from $job->getData(); update the
loop in GetCommand (the foreach over $data and $output->writeln call) to
serialize non-scalar values before outputting: for each $k/$v from
$job->getData() check is_scalar/is_null and print directly, otherwise convert $v
to a readable string (e.g. json_encode or var_export) and then call
$output->writeln("{$k}: " . $serializedValue) so arrays/objects are rendered
safely.
- Around line 24-74: Add a new PHPUnit test file
tests/src/Command/Job/GetCommandTest.php that covers GetCommand behavior: use
Symfony\Component\Console\Tester\CommandTester to execute the GetCommand and
assert outputs for (1) missing --id in text mode (expect failure exit code and
"<error>Missing --id</error>"), (2) missing --id in json mode (expect JSON error
with status "error" and message "Missing --id"), and (3) successful retrieval
cases — mock or stub the Job data source so GetCommand (class GetCommand)
returns a predictable array from Job::getData and assert that --fields filters
keys correctly and that --format=json returns pretty JSON while text format
prints "key: value" lines; ensure tests instantiate GetCommand, pass options
'format', 'id', and 'fields' to CommandTester, and assert exit codes
SUCCESS/FAILURE and exact output content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca7a94a3-2fb0-4586-985e-c8273d5f9a85
📒 Files selected for processing (1)
src/Command/Job/GetCommand.php
- Added an icon for the MultiFlexi CLI in the metainfo XML. - Updated the installation file to include the new SVG icon. - Cleaned up whitespace in CompanyAppCommand and ListCommand classes. - Fixed formatting in InitCommand for better readability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Command/Encryption/InitCommand.php (1)
59-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlgorithm label stored in DB does not match the algorithm actually used — decrypt will fail.
Line 59 encrypts with
'aes-256-cbc', but line 72 records'aes-256-gcm'in thealgorithmcolumn. Any code that later reads this column to choose the decryption cipher will attempt GCM decryption on a CBC ciphertext, producing a decryption failure or corrupted plaintext. The two must be consistent.🐛 Proposed fix — align the stored algorithm with the actual cipher used
Option A — keep CBC and fix the label:
- $encryptedKey = openssl_encrypt($key, 'aes-256-cbc', $hashedMasterKey, \OPENSSL_RAW_DATA, $iv); + $encryptedKey = openssl_encrypt($key, 'aes-256-cbc', $hashedMasterKey, \OPENSSL_RAW_DATA, $iv); // ... - VALUES ('credentials', :key_data, 'aes-256-gcm', NOW(), TRUE) + VALUES ('credentials', :key_data, 'aes-256-cbc', NOW(), TRUE)Option B — switch to GCM (preferred; authenticated encryption, no separate IV storage needed) and fix the label to match:
- $iv = random_bytes(16); - $hashedMasterKey = hash('sha256', $masterKey, true); - $encryptedKey = openssl_encrypt($key, 'aes-256-cbc', $hashedMasterKey, \OPENSSL_RAW_DATA, $iv); + $iv = random_bytes(12); // GCM standard nonce size + $tag = ''; + $hashedMasterKey = hash('sha256', $masterKey, true); + $encryptedKey = openssl_encrypt($key, 'aes-256-gcm', $hashedMasterKey, \OPENSSL_RAW_DATA, $iv, $tag);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Encryption/InitCommand.php` around lines 59 - 72, The DB algorithm label must match the cipher used: either change the openssl_encrypt call in InitCommand.php to use 'aes-256-gcm' and update how you persist key_data (capture the GCM auth tag from openssl_encrypt and store iv|ciphertext|tag in $keyData before inserting, and set the INSERT algorithm value to 'aes-256-gcm'), or keep 'aes-256-cbc' by changing the INSERT algorithm value to 'aes-256-cbc' so it matches the existing openssl_encrypt call; locate the openssl_encrypt usage (producing $encryptedKey and $iv), the $keyData construction, and the INSERT into encryption_keys to make the consistent change and ensure any decrypt logic that reads the algorithm column is updated to expect the chosen format (CBC: iv|ciphertext; GCM: iv|ciphertext|tag).
🧹 Nitpick comments (1)
src/Command/Encryption/InitCommand.php (1)
22-32: ⚡ Quick winMissing docblocks on class and methods.
InitCommand,configure(), andexecute()have no PHPDoc blocks. As per coding guidelines, every class and function must include a docblock describing its purpose, parameters, and return types.📝 Proposed docblocks
+/** + * Initializes or re-initializes the AES encryption key used for storing credentials. + */ class InitCommand extends BaseCommand { protected static $defaultName = 'encryption:init'; + /** + * Configures the encryption:init command options. + */ protected function configure(): void {+ /** + * Executes the encryption key initialization. + * + * `@param` InputInterface $input Console input + * `@param` OutputInterface $output Console output + * + * `@return` int Command exit code (SUCCESS or FAILURE) + */ protected function execute(InputInterface $input, OutputInterface $output): int {As per coding guidelines: "Include a docblock for every function and class, describing its purpose, parameters, and return types."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/Encryption/InitCommand.php` around lines 22 - 32, Add PHPDoc blocks for the InitCommand class and its methods: place a class-level docblock above class InitCommand describing its purpose (initializing/re-initializing encryption keys), add a docblock above configure() describing what it configures, and add a docblock above execute() specifying parameters (`@param` InputInterface $input, `@param` OutputInterface $output) and return type (`@return` int) plus a short description of the action and behavior; use standard PHPDoc tags and exact type names (InputInterface, OutputInterface, int) so tooling and static analysis pick them up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composer.json`:
- Line 46: Replace the wildcard PHPUnit constraint to avoid unpredictable major
upgrades: in composer.json update the "phpunit/phpunit": "*" entry to a bounded
major compatible with PHP 8.1/Symfony 6.4 (for example "phpunit/phpunit": "^11")
so CI and local installs remain deterministic; ensure you run composer update
and commit the changed composer.json and composer.lock.
---
Outside diff comments:
In `@src/Command/Encryption/InitCommand.php`:
- Around line 59-72: The DB algorithm label must match the cipher used: either
change the openssl_encrypt call in InitCommand.php to use 'aes-256-gcm' and
update how you persist key_data (capture the GCM auth tag from openssl_encrypt
and store iv|ciphertext|tag in $keyData before inserting, and set the INSERT
algorithm value to 'aes-256-gcm'), or keep 'aes-256-cbc' by changing the INSERT
algorithm value to 'aes-256-cbc' so it matches the existing openssl_encrypt
call; locate the openssl_encrypt usage (producing $encryptedKey and $iv), the
$keyData construction, and the INSERT into encryption_keys to make the
consistent change and ensure any decrypt logic that reads the algorithm column
is updated to expect the chosen format (CBC: iv|ciphertext; GCM:
iv|ciphertext|tag).
---
Nitpick comments:
In `@src/Command/Encryption/InitCommand.php`:
- Around line 22-32: Add PHPDoc blocks for the InitCommand class and its
methods: place a class-level docblock above class InitCommand describing its
purpose (initializing/re-initializing encryption keys), add a docblock above
configure() describing what it configures, and add a docblock above execute()
specifying parameters (`@param` InputInterface $input, `@param` OutputInterface
$output) and return type (`@return` int) plus a short description of the action
and behavior; use standard PHPDoc tags and exact type names (InputInterface,
OutputInterface, int) so tooling and static analysis pick them up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa3a9c53-98b8-45e8-aa2f-6f782e02b557
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockmultiflexi-cli.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
composer.jsondebian/cz.vitexsoftware.multiflexi-cli.metainfo.xmldebian/multiflexi-cli.installsrc/Command/CompanyAppCommand.phpsrc/Command/Encryption/InitCommand.phpsrc/Command/Job/ListCommand.php
💤 Files with no reviewable changes (1)
- src/Command/CompanyAppCommand.php
✅ Files skipped from review due to trivial changes (2)
- debian/cz.vitexsoftware.multiflexi-cli.metainfo.xml
- src/Command/Job/ListCommand.php
Replaces "*" with "^13" to prevent unpredictable major upgrades. Matches the version already resolved in the lock file (PHP 8.4 env). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
--fieldsoption was used inexecute()ofjob:getbut never declared inconfigure(), causing the error:The "fields" option does not exist.->addOption('fields', ...)toGetCommand::configure()to match the existing usageTest plan
multiflexi-cli job:get --id <id>— should work without errormultiflexi-cli job:get --id <id> --fields name,status— should filter output to specified fields🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
job:getcommand accepts an optional--fieldsparameter to display only the specified job attributes (comma-separated) in both JSON and text output.Chores