fix(change): integrate CSV path resolution into main#1777
Conversation
fix(change): resolve CSV code paths from prompt subdirs
gltanaka
left a comment
There was a problem hiding this comment.
Thanks for the update. I verified the current head (080fc23965250a21c157b8606cd61cb23dbe0bed) and CI is green, but I do not think this is ready to merge yet.
Blocking issue: resolve_code_path preserves the prompt directory under code_directory verbatim. That works for the synthetic test layout, but it fails a realistic PDD prompt-root layout. For example, with INPUT_CODE=pdd and a CSV prompt path like prompts/commands/modify_python.prompt (or a full path containing pdd/prompts/commands/...), the resolver tries pdd/prompts/commands/modify.py and then falls back to pdd/modify.py. The actual paired code file is pdd/commands/modify.py, and architecture.json maps commands/modify_python.prompt to pdd/commands/modify.py.
Concrete repro against this branch:
resolve_code_path(
prompt_name="prompts/commands/modify_python.prompt",
resolved_prompt_path="$repo/pdd/prompts/commands/modify_python.prompt",
csv_file="$repo/changes.csv",
code_directory="$repo/pdd",
input_code_filename="modify.py",
)
# returns: $repo/pdd/modify.py (does not exist)
# expected: $repo/pdd/commands/modify.pyThe added regression test encodes code/prompts/pkg/widget.py, which does not match this repo's pdd/prompts/<subdir> to pdd/<subdir> convention. Please either strip recognized prompt roots such as prompts/ or <package>/prompts/ before joining under INPUT_CODE, or reuse the existing architecture/path metadata, and add a regression for a real PDD-style case like pdd/prompts/commands/foo_python.prompt -> pdd/commands/foo.py.
Also, the README now says code lookup is constrained to remain inside INPUT_CODE, but _inside_code_directory uses abspath/commonpath on the directory string before opening the candidate. A symlinked subdirectory under INPUT_CODE can still point outside and be returned. If that containment guarantee is intentional, compare realpath/resolved paths for both the code directory and the final candidate before returning it.
Validation note: I ran the focused pytest and adjacent command-layer tests locally and they pass, but they do not cover the prompt-root layout above. A live provider e2e is not necessary here; a focused pdd change --manual --csv or change_main test with real nested prompt/code files and a mocked LLM boundary is feasible and would cover the user-facing path.
gltanaka
left a comment
There was a problem hiding this comment.
I re-reviewed the updated head 3a6c438fa119ba63d5e95082172f82569645bf70.
The prior blockers are addressed:
- PDD prompt-root paths such as
prompts/commands/foo_python.promptandpdd/prompts/commands/foo_python.promptnow resolve to the paired code path underpdd/commands/foo.pyrather than preserving the literalprompts/directory or falling back flat. - Code lookup now checks the resolved final candidate path against the resolved code directory, so symlinked candidate directories outside
INPUT_CODEare skipped. - Regression coverage now includes the real PDD prompt-root layout, the symlink containment case, and a
change_mainCSV path with a mocked LLM boundary.
Validation checked:
- GitHub checks are green: CodeQL, auto-heal, Public CLI Regression, Package Preprocess Smoke, and Run Unit Tests.
- Local targeted tests pass:
python -m pytest tests/test_process_csv_change.py -qandpython -m pytest tests/test_change_main.py tests/test_commands_modify.py tests/commands/test_modify.py -q. python -m py_compile pdd/process_csv_change.pypasses.
A live provider e2e run is not necessary for this change. The changed behavior is deterministic path selection before the LLM call, and the new change_main test exercises that user-facing batch-change path while mocking the LLM boundary to avoid credentials, cost, and nondeterminism.
Summary
Validation