Harden skill evaluation and repository controls#5
Conversation
dd3ok
force-pushed
the
codex/best-practice-hardening
branch
from
4d897a3 to
0b6498d
Compare
![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 provider-neutral skill activation evaluation harness (run_skill_eval.py) with corresponding tests, and updates the repository's documentation, workflows, and verification scripts to support this protocol. It also refines the semantic rules and guard catalogs for the boring-backend skill. Feedback on these changes suggests wrapping path operations in command_file_hashes with a try-except OSError block to prevent crashes on Windows when processing arguments with invalid filename characters. Additionally, it is recommended to make the evaluation harness more resilient by recording individual trial failures rather than aborting the entire run upon a timeout or non-zero exit code.
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 command_file_hashes(command: list[str]) -> list[dict[str, Any]]: | ||
| files = [] | ||
| for index, argument in enumerate(command): | ||
| candidate = Path(argument).expanduser() | ||
| if not candidate.is_absolute(): | ||
| candidate = ROOT / candidate | ||
| if not candidate.is_file() and index == 0: | ||
| executable = shutil.which(argument) | ||
| if executable is not None: | ||
| candidate = Path(executable) | ||
| if candidate.is_file(): | ||
| resolved = candidate.resolve() | ||
| files.append( | ||
| { | ||
| "argument_index": index, | ||
| "argument": argument, | ||
| "path": str(resolved), | ||
| "sha256": hash_file(resolved), | ||
| } | ||
| ) | ||
| return files |
There was a problem hiding this comment.
On Windows, Path(argument).expanduser() and candidate.is_file() can raise an OSError if the argument contains characters that are invalid in Windows filenames (such as <, >, |, *, ?). Since command can contain arbitrary arguments passed to the runner (including query strings, JSON payloads, or other non-path strings), this can cause the evaluation to crash on Windows. Wrapping the path operations in a try...except OSError: block will make the file hashing robust and cross-platform.
def command_file_hashes(command: list[str]) -> list[dict[str, Any]]:
files = []
for index, argument in enumerate(command):
try:
candidate = Path(argument).expanduser()
if not candidate.is_absolute():
candidate = ROOT / candidate
if not candidate.is_file() and index == 0:
executable = shutil.which(argument)
if executable is not None:
candidate = Path(executable)
if candidate.is_file():
resolved = candidate.resolve()
files.append(
{
"argument_index": index,
"argument": argument,
"path": str(resolved),
"sha256": hash_file(resolved),
}
)
except OSError:
continue
return files| if completed.timed_out: | ||
| cleanup = ( | ||
| f"; cleanup best effort: {'; '.join(completed.cleanup_errors)}" | ||
| if completed.cleanup_errors | ||
| else "" | ||
| ) | ||
| raise EvalError(f"runner timed out for {run_id}{detail}{cleanup}") | ||
| if completed.cleanup_errors: | ||
| raise EvalError( | ||
| f"runner cleanup failed for {run_id}: {'; '.join(completed.cleanup_errors)}" | ||
| ) | ||
| if completed.returncode != 0: | ||
| raise EvalError( | ||
| f"runner exited {completed.returncode} for {run_id}{detail}" | ||
| ) |
There was a problem hiding this comment.
Aborting the entire evaluation run on the first timeout or non-zero exit code of an adapter makes the harness highly fragile. Transient network issues or timeouts are common when calling LLM providers. Consider recording the failure for the specific trial in results.jsonl (e.g., with a failed status or null response) and continuing the evaluation loop so that a single transient issue does not discard the results of the entire suite.
1 participant
Summary
Verification
python scripts/verify_all.py(77 tests; POSIX-only test skipped on Windows).agents, and.claudepackages passquick_validate.pypython -m ruff check scripts testspython -m py_compile ...gh skill publish --dry-run .fromskills/nullChecklist
skills/boring-backend/..agents/skills/boring-backend/and.claude/skills/boring-backend/are synchronized.python scripts/verify_all.pypasses.