Improve savepoint input hygiene and direct save flags#25
Conversation
![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 introduces a new validation script check-output-contract.py to verify the output contract JSON, adds support for direct CLI flags in savepoint.py as an alternative to input JSON files, and implements pre-render redaction scanning along with automatic input deletion on success. Feedback on these changes highlights opportunities to improve the robustness of path validation in is_under_savepoint_dir to safely handle symlinks and uncaught OSError exceptions, catch broader OSError exceptions during contract validation, and correct a minor typo in an error message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def is_under_savepoint_dir(path: Path) -> bool: | ||
| base = (Path.cwd() / ".savepoint").resolve() | ||
| absolute = path if path.is_absolute() else Path.cwd() / path | ||
| try: | ||
| absolute.resolve().relative_to(base) | ||
| except ValueError: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
The path validation in is_under_savepoint_dir resolves the input path using absolute.resolve() before checking if it is under .savepoint/. If the input path is a symlink located outside .savepoint/ that points to a file inside .savepoint/, absolute.resolve() will resolve to the path inside .savepoint/, causing the function to return True. Consequently, delete_input_on_success will delete the symlink located outside .savepoint/, violating the safety guarantee.\n\nAdditionally, absolute.resolve() can raise an OSError (e.g., due to permission issues or infinite symlink loops), which is currently uncaught and could crash the CLI.\n\nTo make this robust and secure:\n1. Catch OSError along with ValueError.\n2. Verify that the parent directory of the unresolved path is under .savepoint/ (by resolving absolute.parent) to ensure the symlink/file itself resides inside .savepoint/.\n3. Ensure the resolved path is a file and also resides under .savepoint/.
def is_under_savepoint_dir(path: Path) -> bool:\n base = (Path.cwd() / ".savepoint").resolve()\n absolute = path if path.is_absolute() else Path.cwd() / path\n try:\n parent_resolved = absolute.parent.resolve()\n parent_resolved.relative_to(base)\n resolved = absolute.resolve()\n resolved.relative_to(base)\n return resolved.is_file()\n except (ValueError, OSError):\n return False| def validate_contract(path: Path) -> list[str]: | ||
| errors: list[str] = [] | ||
| try: | ||
| data = json.loads(path.read_text(encoding="utf-8-sig")) | ||
| except FileNotFoundError: | ||
| return [f"{path}: file does not exist"] | ||
| except json.JSONDecodeError as exc: | ||
| return [f"{path}: invalid JSON: {exc}"] |
There was a problem hiding this comment.
In validate_contract, only FileNotFoundError and json.JSONDecodeError are caught when reading the contract file. If any other OSError occurs (such as PermissionError or IsADirectoryError), the script will crash with an unhandled exception instead of returning a clean list of errors.\n\nCatching OSError instead of FileNotFoundError makes the validation script much more robust.
def validate_contract(path: Path) -> list[str]:\n errors: list[str] = []\n try:\n data = json.loads(path.read_text(encoding="utf-8-sig"))\n except OSError as exc:\n return [f"{path}: failed to read file: {exc}"]\n except json.JSONDecodeError as exc:\n return [f"{path}: invalid JSON: {exc}"]| elif any(not isinstance(item, str) or not item.strip() for item in must): | ||
| errors.append(f"{path}: case #{index} must entries must be non-empty strings") |
There was a problem hiding this comment.
There is a minor typo/grammatical issue in the error message on line 86: "must entries must be". It should be clarified to refer to the 'must' field/entries specifically.
elif any(not isinstance(item, str) or not item.strip() for item in must):\n errors.append(f"{path}: case #{index} 'must' entries must be non-empty strings")1 participant
Summary
--scan-redactionbefore rendering, without printing matched secret values.saveflags for simple savepoints and safe--delete-input-on-successcleanup under.savepoint/.check-output-contract.py, wire it into validation, and document migration/input/Windows notes in README and references.Validation
python3 scripts/check-frontmatter.pypython3 scripts/check-marker-block.pypython3 scripts/check-marker-semantics.pypython3 scripts/validate-examples.pypython3 scripts/check-output-contract.pypython3 scripts/validate-repo.pypython3 scripts/check-savepoint-renderer.pypython3 scripts/check-install-helper.py(Windows symlink case skipped due local privilege)python3 scripts/savepoint.py validate --allow-example-paths examples/SAVEPOINT.filled.example.md examples/file-bugfix/SAVEPOINT.md examples/file-architecture/SAVEPOINT.md examples/unsafe-savepoint/SAVEPOINT.mdpython3 -m compileall -q skills/savepoint/scripts scriptsgit diff --check