Quality pass + PR #6 review fixes#7
Conversation
…ted to execution-log schema, reject path-like identifiers earlier, verifier now content-checks prefetch/registry/evtx artifacts; QUALITY_REVIEW.md
…_output_bytes, configurable timeout_seconds, drop unpopulated record_offset (use record_number), pinned/readable parser resolution, hash handling, portable test shebangs; verifier content-checks + schema/identifier hardening
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b6cd86714
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with stdout_path.open("wb") as stdout_handle, stderr_path.open( | ||
| "wb" | ||
| ) as stderr_handle: |
There was a problem hiding this comment.
When a caller reuses a tool-run ID whose output directory contains evtx_dump_stdout.xml or evtx_dump_stderr.txt as a symlink, opening these paths directly with "wb" follows the symlink and truncates its target. In particular, a symlink to registered evidence silently modifies that evidence after the hash check, bypassing ToolContext.write_json and ensure_not_registered_evidence_path; validate the resolved destinations or create the files without following symlinks before writing.
Useful? React with 👍 / 👎.
| program_names = { | ||
| str(program.get("program_name", "")).lower() | ||
| for program in artifact.get("programs", []) | ||
| if program.get("program_name") |
There was a problem hiding this comment.
Because artifact payloads are not schema-validated, a successful plugin tool can produce a Prefetch artifact such as {"artifact_type":"prefetch","programs":["bad"]}. The verifier then calls .get() on the string here and raises AttributeError instead of deterministically rejecting the unsupported artifact; validate collection and record types before dereferencing them. The Registry Run key and EVTX branches have the same malformed-record risk.
Useful? React with 👍 / 👎.
| if not content_supported: | ||
| notes.append(support_note) | ||
| notes.append(content_note) | ||
| return self._decision(candidate, "rejected", "rejected", notes) |
There was a problem hiding this comment.
When this new content check rejects a supported artifact type because an executable or event ID is absent, SelfCorrectionLoop cannot correct it: detect_missing_support only calls artifact_support_decision, sees that the artifact type supports the claim type, returns trigger none, and selects no follow-up action. For example, a notepad.exe claim sourced from an rclone.exe Prefetch artifact now stops immediately instead of rerunning the targeted Prefetch tool for notepad.exe; expose a content-mismatch trigger to the correction loop.
Useful? React with 👍 / 👎.
| returncode = process.poll() | ||
| if stdout_path.exists() and stdout_path.stat().st_size > max_output_bytes: |
There was a problem hiding this comment.
When evtx_dump emits large diagnostics to stderr, only the stdout file size is checked against max_output_bytes, while stderr is continuously written to an uncapped file until the potentially hour-long timeout. A malformed EVTX or failing parser that logs rapidly can therefore exhaust the analysis filesystem despite the configured output cap; monitor and cap stderr as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Follow-up quality pass on PR #6’s real EVTX typed tool and surrounding workflow: strengthens identifier validation, tightens evtx_dump resolution/execution guardrails, and adds deterministic verifier content checks with accompanying schema + test updates.
Changes:
- Add identifier validation for
case_id,evidence_id, andtool_run_id, and extend schemas/tests to cover path-like identifiers. - Harden
parse_evtx_realexecution (rehash check, trusted parser resolution, streamed stdout with output/timeout limits) and update docs/tests accordingly. - Add deterministic verifier “content-consistency” checks (Prefetch/RunKey/EVTX) and validate execution-log events against schema in reporting/audit-package tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_typed_tools.py | Adds regression test ensuring tool_run_id rejects path-like values. |
| tests/test_reporting.py | Validates every execution-log event against the execution-log schema. |
| tests/test_parse_evtx_real.py | Updates EVTX tests for new parser resolution + output/timeout/hash-mismatch behaviors. |
| tests/test_findings_verifier.py | Adds a regression test rejecting Prefetch claims for executables absent from artifact content. |
| tests/test_evidence_registration.py | Adds tests that case_id / evidence_id reject path-like identifiers. |
| src/sift_crs/tools.py | Refactors EVTX parser discovery/execution and adds streaming + limits + hash recheck + tool_run_id validation. |
| src/sift_crs/findings.py | Adds claim/artifact content-consistency checks and helper extractors. |
| src/sift_crs/evidence.py | Adds shared identifier validation and enforces it for case_id and evidence_id. |
| schemas/tool-run.schema.json | Tightens tool_run_id schema with an identifier pattern. |
| schemas/parse_evtx_real.input.schema.json | Adds typed inputs for timeout_seconds and max_output_bytes. |
| schemas/execution-log.schema.json | Adds missing report_generated / audit_package_exported event types. |
| README.md | Updates EVTX parser documentation to reflect new guardrails/limits and record fields. |
| docs/QUALITY_REVIEW.md | Adds per-issue quality review notes and responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def validate_identifier(value: str, field_name: str) -> None: | ||
| if not isinstance(value, str) or not value or IDENTIFIER_RE.fullmatch(value) is None: | ||
| raise ValueError( | ||
| f"{field_name} must match ^[A-Za-z0-9._:-]+$ and must not be empty" | ||
| ) |
| def _extract_executable_names(text: str) -> set[str]: | ||
| return {match.lower() for match in re.findall(r"\b[A-Za-z0-9_.-]+\.exe\b", text)} |
Follow-up to #6. High-reasoning quality pass plus resolution of the Copilot and Codex review comments on the real EVTX parser. 48 passed, 1 skipped. See docs/QUALITY_REVIEW.md for the per-comment responses.