Add --reverse to fedify lookup output ordering#609
Conversation
Implement the new --reverse option for fedify lookup and apply it across default multi-input lookup, recurse chains, and traversal output while preserving existing error and partial-output behavior. Traversal now keeps streaming semantics when --reverse is disabled, and reverse mode preserves already-fetched items even if traversal fails later. Recurse mode also preserves root-object emission when a later recursive fetch fails. Update CLI docs and lookup config documentation, add changelog entries, and expand lookup tests for option parsing, ordering, and partial-collection behavior. Fixes fedify-dev#607 Co-Authored-By: OpenAI Codex <codex@openai.com>
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 enhances 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
|
--reverse to fedify lookup output ordering
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the presentation order of results for multi-input lookups, collection traversals, and recursive chains. The implementation correctly handles the necessary buffering for reversal while preserving existing streaming behavior when the option is not used. The changes are well-documented and include thorough tests.
There was a problem hiding this comment.
Pull request overview
Adds a --reverse presentation option to fedify lookup so users can invert the output ordering of results across default multi-input lookups, --traverse collection traversal, and --recurse chain output, without changing lookup semantics.
Changes:
- Added
--reverseCLI parsing + config binding (lookup.reverse) and config schema support. - Implemented presentation-order helpers and applied reversal to recurse/traverse/default output paths.
- Updated CLI docs and changelog to document the new option.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/lookup.ts | Adds --reverse option, introduces ordering/buffering helpers, and applies reversed presentation logic across modes. |
| packages/cli/src/lookup.test.ts | Adds tests for --reverse parsing/config and helper behavior (toPresentationOrder, collectAsyncItems). |
| packages/cli/src/config.ts | Extends lookup config schema with optional reverse boolean. |
| docs/cli.md | Documents --reverse and adds reverse = false to lookup config example. |
| CHANGES.md | Records the new --reverse option in the unreleased changelog. |
Comments suppressed due to low confidence (1)
packages/cli/src/lookup.ts:1204
- The default multi-URL lookup path now always buffers into
successfulObjectsand prints in a second pass, even whencommand.reverseis false. That changes the write/error behavior vs the prior one-pass write-in-loop (e.g., write failures occur after all spinner statuses are emitted) and adds an unnecessary extra pass. Consider only buffering whencommand.reverseis true; otherwise keep the original one-pass write flow.
const successfulObjects: APObject[] = [];
for (const [i, obj] of objects.entries()) {
const url = command.urls[i];
if (obj == null) {
spinner.fail(`Failed to fetch ${colors.red(url)}`);
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Add runLookup-level tests that verify emitted output order for multi-input default lookup, recurse mode, and traverse mode when --reverse is enabled. To keep these tests deterministic and network-free, runLookup now accepts injectable lookup/traverse dependencies with defaults bound to the existing implementations. Production behavior is unchanged. The new tests execute the real runLookup control flow, capture output, and assert final ordering in each mode. fedify-dev#609 (comment) Co-Authored-By: OpenAI Codex <codex@openai.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order of results. The feature is implemented across all relevant modes: default multi-input, collection traversal (--traverse), and recursive chain lookup (--recurse). The implementation correctly handles buffering when necessary (for traversal and multi-input modes) while preserving existing streaming behavior and failure semantics. The changes are well-tested, with new unit and integration tests covering the reverse ordering logic and error handling. Documentation and changelog have also been updated accordingly. The implementation appears solid and well-executed.
Restore recurse-mode output timing so the root object is emitted immediately when --reverse is not used, preserving prior streaming/visibility semantics. Also make runLookup accept an injected exit dependency and update the runLookup ordering tests to use that injection instead of global process.exit monkeypatching. fedify-dev#609 (comment) fedify-dev#609 (comment) Co-Authored-By: OpenAI Codex <codex@openai.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order for various lookup modes. However, it introduces a potential memory exhaustion vulnerability when used with the --traverse option, as the tool buffers the entire collection in memory, which can lead to crashes with large ActivityPub collections.
Avoid an unnecessary array clone in toPresentationOrder() for non-reverse paths when input is already an array, and tighten the --reverse docs wording for clarity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order of results. The changes are well-implemented across the default, traverse, and recurse modes, with thoughtful consideration for failure semantics to preserve partial results. The documentation and tests have been updated accordingly, making this a solid feature addition. I have one minor suggestion to improve the consistency of a helper function.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, enabling users to reverse the output order of results. The implementation correctly handles this for default, traverse, and recurse modes, preserving streaming for non-reversed cases while buffering for reversed ones. The changes are well-documented and thoroughly tested.
Add runLookup integration tests for reverse-mode failure paths: traverse+reverse now verifies reversed partial item emission with non-zero exit, and recurse+reverse verifies root object emission on recursive failure.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order of results. The implementation correctly handles this for default multi-URL lookups, recursive lookups, and collection traversals. The changes also include updates to documentation, configuration, and comprehensive tests for the new functionality.
I've found one issue in the implementation for collection traversal (--traverse) where the logic for printing separators between items has a regression. This affects both normal and reversed traversal modes. My review includes a detailed comment and a code suggestion to fix this, aligning with the rule on managing state for independent processing chains.
Add runLookup regression coverage to verify separators are emitted between adjacent traversed items in both normal and reverse traversal modes. This locks in the current behavior and prevents false-positive regressions around same-collection item boundaries. fedify-dev#609 (comment) Co-Authored-By: Codex <codex@openai.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order of results. The implementation correctly handles this new option across default, traverse, and recurse modes, including failure scenarios. The changes are well-tested and include necessary documentation updates. I have one suggestion to improve the readability of the recursive lookup logic.
Avoid extra array cloning in reverse output paths by iterating buffered entries in reverse order for recurse and traverse modes. Also tighten test helper typing by replacing the direct command-object type assertion with a `satisfies`-checked base command shape. fedify-dev#609 (comment) fedify-dev#609 (comment) fedify-dev#609 (comment) Co-Authored-By: Codex <codex@openai.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order of results. The changes are well-implemented across default, traverse, and recurse modes, with thorough test coverage for the new functionality and its edge cases. The documentation has also been updated accordingly. I have one suggestion to improve the readability of the new logic in runLookup.
Keep runLookupAndCaptureExitCode() fully typed by accepting the exact runLookup command parameter type, and move the conversion responsibility into createLookupRunCommand(). fedify-dev#609 (comment) Co-Authored-By: Codex <codex@openai.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --reverse option to the fedify lookup command, allowing users to reverse the output order of results. A security audit found no vulnerabilities meeting the severity threshold. The implementation is well-tested and documented.
Summary
This PR adds a new
--reverseoption tofedify lookupso users can reverse the presentation order of emitted results without changing lookup semantics across default multi-input lookup,--traversecollection output, and--recursechain output.Changes
Added
--reverseoption parsing to the lookup command, addedlookup.reversesupport in the config schema, and applied reverse ordering logic to default multi-input output, recurse chain output, and traversal output.Preserved existing behavior and failure semantics by keeping streaming traversal when
--reverseis not enabled, preserving already-fetched items in--traverse --reversewhen traversal fails later, and preserving root-object emission in recurse-mode failure paths after the root object has been fetched.Updated documentation in docs/cli.md by adding a
--reverseoption section and addingreverse = falseto the[lookup]config example, and updated changelog entries in CHANGES.md.Tests
Added parser/config tests for
--reverse, added ordering helper tests, and added async collection tests to verify partial-item preservation on iterator failure, all in packages/cli/src/lookup.test.ts.Ran
deno test --allow-all packages/cli/src/lookup.test.ts, and all tests passed.References
Closes #607