Repository navigation
fix(ingest): treat existing files with glob characters in their names as literal paths - #2829
pcbeingused333 wants to merge 1 commit into
Conversation
… as literal paths Files such as 'Invoice [2024].pdf' were dropped in service mode because the already-resolved path was globbed again, and a directory holding both 'report1.txt' and 'report[1].txt' returned report1.txt twice and lost the bracketed file. Check for an existing file first and only glob otherwise. Fixes NVIDIA#2808 Signed-off-by: Alex Castillo González <alex.castillog33@gmail.com>
[Medium impact] The change reaches file selection across several ingest routes. The path fix appears sound, but the new test file must meet the repository's copyright-year and naming requirements before merging. FindingsFix with agent prompt### Issue 1
nemo_retriever/tests/test_literal_path_glob.py:1
The new test file uses `2024-25` in its copyright header. The repository requires the current year for new Python files, so this header does not meet that requirement. Update it to include `2026`.
```suggestion
# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES.
```
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
### Issue 2
nemo_retriever/tests/test_literal_path_glob.py:13
Both new filename tests use `f` for the file path, here and on line 19. The repository requires descriptive variable names and allows single-letter names only for loop counters or lambda parameters. Rename both variables to `file_path` before merging to meet that requirement.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryExisting files now bypass glob expansion in both helpers, preserving names with glob characters.
pcbeingused333 explicitly describes existing-file precedence over a matching glob as intentional in the PR description. Reviews (1) · Last reviewed commit: "fix(ingest): treat existing files with g..." · Reviewed by Greptile |
| @@ -0,0 +1,38 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2024-25, NVIDIA CORPORATION & AFFILIATES. | |||
There was a problem hiding this comment.
The new test file uses 2024-25 in its copyright header. The repository requires the current year for new Python files, so this header does not meet that requirement. Update it to include 2026.
| # SPDX-FileCopyrightText: Copyright (c) 2024-25, NVIDIA CORPORATION & AFFILIATES. | |
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. |
Rule Used: Python files added in this PR must include the SPDX license header at the top: # SPDX-FileCopyrightText: Copyright (c) <year>, NVIDIA CORPORATION & AFFILIATES. # All rights reserved. # SPDX-License-Identifier: Apache-2.0 Use the current year for ne... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/tests/test_literal_path_glob.py
Line: 1
Comment:
**Copyright year is outdated**
The new test file uses `2024-25` in its copyright header. The repository requires the current year for new Python files, so this header does not meet that requirement. Update it to include `2026`.
```suggestion
# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES.
```
**Rule Used:** Python files added in this PR must include the SPDX license header at the top: # SPDX-FileCopyrightText: Copyright (c) <year>, NVIDIA CORPORATION & AFFILIATES. # All rights reserved. # SPDX-License-Identifier: Apache-2.0 Use the current year for ne... ([source](https://github.com/nvidia/nemo-retriever/blob/cc1db1b2ba66615d71c20c615a81f7b01211c6b0/.greptile/config.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
|
|
||
| def test_service_keeps_existing_file_with_glob_chars_in_name(tmp_path: Path) -> None: | ||
| f = tmp_path / "Invoice [2024].pdf" |
There was a problem hiding this comment.
Path variables have unclear names
Both new filename tests use f for the file path, here and on line 19. The repository requires descriptive variable names and allows single-letter names only for loop counters or lambda parameters. Rename both variables to file_path before merging to meet that requirement.
Rule Used: Variables, functions, classes, and modules must have descriptive names. Single-letter variables are acceptable only as loop counters or lambda parameters. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/tests/test_literal_path_glob.py
Line: 13
Comment:
**Path variables have unclear names**
Both new filename tests use `f` for the file path, here and on line 19. The repository requires descriptive variable names and allows single-letter names only for loop counters or lambda parameters. Rename both variables to `file_path` before merging to meet that requirement.
**Rule Used:** Variables, functions, classes, and modules must have descriptive names. Single-letter variables are acceptable only as loop counters or lambda parameters. ([source](https://github.com/nvidia/nemo-retriever/blob/cc1db1b2ba66615d71c20c615a81f7b01211c6b0/.greptile/config.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
Closes #2808
Input paths that are already concrete files were passed through
glob.globa second time, so a file whose name contains glob metacharacters ([ ] * ?) was treated as a pattern:expand_service_file_patternsreturned nothing forInvoice [2024].pdf, so the file was silently dropped in service mode.expand_input_file_patternsmatchedreport[1].txtagainstreport1.txt, so a directory containing both returnedreport1.txttwice and lostreport[1].txt.Both helpers now keep a path that is an existing file as a literal path and only glob otherwise, as suggested in the issue. User-supplied patterns are not escaped (see #1076), so
docs/**/*.pdfand intentional bracket patterns behave as before.New
tests/test_literal_path_glob.pycovers a bracketed file name in service and plain expansion, a directory withreport1.txtplusreport[1].txt, and a check that*.pdfstill expands. The service and directory cases fail on main and pass with the change; black (line length 120) and flake8 from pre-commit are clean on the touched files.This change was prepared with AI assistance (Claude Code).
Checklist