feat(sandbox): restack exact patch quarantine after nanoid - #93
Closed
seonghobae wants to merge 3 commits into
Closed
feat(sandbox): restack exact patch quarantine after nanoid#93seonghobae wants to merge 3 commits into
seonghobae wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthrough정확한 저장소·커밋·패치 다이제스트에 바인딩된 패치 검증기를 추가했다. Git 소스와 exact tree를 검증하고, archive와 추출 결과를 재검증한다. Docker sandbox는 네트워크·권한·자원·결과 채널을 제한한다. 관련 회귀 테스트와 운영 문서를 추가했다. Changes패치 격리 검증
Estimated code review effort: 5 (Critical) | ~90 minutes Mergeability Score: 🔵 Low · up to The PR adds exact patch-quarantine validation and currently passes its stated checks, but valid patches containing unquoted spaces may be rejected, permission-related metadata errors may escape the expected error contract, and large archives may incur avoidable quadratic processing. It is mergeable with explicit owner awareness and follow-up for these bounded risks. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
marked this pull request as ready for review
August 10, 2026 09:48
This was referenced Aug 10, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
reviewer/tests/test_patch_validation_streaming_edges.py (2)
413-433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win부분 레코드 조립 결과를 단정하십시오.
이 테스트는 예외가 없는 것만 확인합니다. 반환된 인벤토리의 경로, 모드, 오브젝트 ID, 크기를 단정하면 청크 경계에서 필드가 잘려도 검출됩니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/tests/test_patch_validation_streaming_edges.py` around lines 413 - 433, Update test_consume_exact_tree_stream_accepts_partial_records to capture the return value from _consume_exact_tree_stream and assert the reconstructed inventory’s path, mode, object ID, and size, ensuring fields split across chunk boundaries are assembled correctly.
436-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win무한 반환 가짜 리더를 유한 반복자로 바꾸십시오.
_read_git_stream_chunk대체 함수가 항상b"12345"를 반환합니다. 레코드 상한 검사가 회귀하면_consume_exact_tree_stream이 종료되지 않고 테스트가 CI에서 멈춥니다. 유한 반복자를 쓰면 회귀 시 즉시 실패합니다.♻️ 제안 수정
process = _FakeProcess() monkeypatch.setattr(patch_validation, "MAX_SOURCE_TREE_RECORD_BYTES", 4) + chunks = iter((b"12345",) * 4 + (b"",)) monkeypatch.setattr( patch_validation, "_read_git_stream_chunk", - lambda *_args, **_kwargs: b"12345", + lambda *_args, **_kwargs: next(chunks), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/tests/test_patch_validation_streaming_edges.py` around lines 436 - 451, Update test_consume_exact_tree_stream_rejects_unterminated_record_ceiling so the _read_git_stream_chunk monkeypatch returns data from a finite iterator rather than always returning b"12345". Keep the first returned chunk sufficient to trigger the record byte-limit error, and ensure subsequent reads are exhausted so a regression cannot cause the test to hang.reviewer/noema_reviewer/patch_validation.py (4)
1402-1415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_read_result_payload의_completed매개변수는 사용되지 않습니다.호출부(Line 1576-1580)는
completed를 전달하지만 함수는 이를 무시합니다. 향후 확장 계획이 없다면 매개변수와 인수를 함께 제거하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/noema_reviewer/patch_validation.py` around lines 1402 - 1415, Remove the unused _completed parameter from _read_result_payload and update its callers, including the call that passes completed, so the function signature and invocation no longer accept or provide this argument.
1455-1458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
metadata_kind is None검사가 중복이며 도달할 수 없습니다.
_verify_source_head는metadata_kind가None이면 같은 메시지로 이미 예외를 발생시킵니다(Line 870-871). 따라서 Line 1457-1458은 실행되지 않습니다. 타입 좁히기 목적이라면 주석을 남기고, 그렇지 않으면 제거하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/noema_reviewer/patch_validation.py` around lines 1455 - 1458, Remove the unreachable metadata_kind is None check after _verify_source_head in the source validation flow, since _verify_source_head already raises for that case. If the check is required solely for type narrowing, replace it with an explanatory comment; otherwise leave the existing _verify_source_head call and subsequent logic unchanged.
1016-1038: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_validated_exact_tree_output는 결과를 버리며 스트리밍 검증기와 로직이 중복됩니다.이 함수는
inventory를 채운 다음 반환하지 않고 폐기합니다. 프로덕션 경로는_consume_exact_tree_stream만 사용합니다. 두 구현이 같은 한계값과 규칙을 따로 유지하면 향후 한쪽만 수정될 위험이 있습니다. 이 함수가 테스트 전용이라면 스트리밍 검증기를 재사용하도록 정리하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/noema_reviewer/patch_validation.py` around lines 1016 - 1038, Update _validated_exact_tree_output to reuse _consume_exact_tree_stream rather than independently splitting records and invoking _validated_exact_tree_record. Preserve the existing validation limits and rules while returning or exposing the validated inventory needed by tests, so the test helper does not maintain a duplicate implementation.
1197-1203: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win빈 디렉터리 검사는 멤버 수에 대해 2차 시간 복잡도로 동작합니다.
declared_directories의 각 항목마다declared_paths전체를 다시 순회합니다. 멤버 상한은MAX_SOURCE_ARCHIVE_MEMBERS = 20_000입니다. 디렉터리가 많은 저장소에서는 최악의 경우 문자열 비교가 수억 회 발생합니다. 부모 경로 집합을 한 번만 만들어 상수 시간으로 조회하십시오.♻️ 제안 수정
- for directory in declared_directories: - prefix = f"{directory}/" - if not any( - path != directory and path.startswith(prefix) - for path in declared_paths - ): - raise ValueError("source archive contains an empty gitlink-like directory") + populated_directories: set[str] = set() + for path in declared_paths: + parent = PurePosixPath(path).parent + while parent != PurePosixPath("."): + populated_directories.add(parent.as_posix()) + parent = parent.parent + for directory in declared_directories: + if directory not in populated_directories: + raise ValueError("source archive contains an empty gitlink-like directory")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/noema_reviewer/patch_validation.py` around lines 1197 - 1203, Optimize the empty-directory validation loop in the declared_directories check by building a set of parent directory paths from declared_paths once, then use constant-time membership checks for each directory instead of repeatedly scanning declared_paths. Preserve the existing path-prefix semantics and ValueError behavior for empty gitlink-like directories.reviewer/tests/test_patch_validation_streaming_final_edges.py (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
subprocess.run을 전역으로 교체하면 테스트 범위가 넓어집니다.
monkeypatch.setattr(patch_validation.subprocess, "run", ...)는 공유된subprocess모듈 속성을 바꿉니다. 테스트 실행 중 다른 코드가subprocess.run을 호출하면 스텁 값을 받습니다. 검증 대상 함수만 감싸도록 좁히십시오. 예를 들어_verify_source_head가 사용하는 호출만 대체하는 헬퍼를 도입하거나, 반환값 검사를 인자별로 구분하십시오.Also applies to: 99-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/tests/test_patch_validation_streaming_final_edges.py` around lines 25 - 29, Replace the global subprocess.run monkeypatch in the affected tests with a narrowly scoped stub for the call used by _verify_source_head. Ensure only that validation path receives the fake return value, while unrelated subprocess.run calls retain their real behavior; apply the same change to both highlighted test locations.reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py (1)
51-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGit 헬퍼가 다른 테스트 파일과 다르게 부분 실행 경로를 사용합니다.
이 파일은
"git"을 직접 호출하고timeout을 지정하지 않습니다. 같은 코호트의test_patch_validation.py와test_patch_validation_security_boundaries.py는patch_validation.TRUSTED_GIT_EXECUTABLE,shell=False,timeout=30을 사용하는_run_git헬퍼를 씁니다. Ruff도 S607(부분 실행 경로)로 이 호출들을 표시합니다. 동일한 헬퍼 형태로 통일하십시오.♻️ 제안 수정
+def _run_git(source: Path, *arguments: str) -> str: + """Run one deterministic non-shell Git command for a test repository.""" + completed = subprocess.run( + [patch_validation.TRUSTED_GIT_EXECUTABLE, "-C", str(source), *arguments], + check=True, + shell=False, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + timeout=30, + ) + return completed.stdout.strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py` around lines 51 - 78, Update the _git_repository helper to run every Git command through the established patch_validation.TRUSTED_GIT_EXECUTABLE-based _run_git pattern, preserving shell=False and timeout=30 for each invocation. Replace direct "git" subprocess calls and reuse the existing shared helper conventions used by the related patch-validation tests.Source: Linters/SAST tools
reviewer/tests/test_patch_validation.py (1)
419-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win성공 경로 테스트가 러너의 실제 UID/GID에 의존합니다.
DockerPatchValidationRunner.validate는os.getuid()또는os.getgid()가 0이면RuntimeError를 발생시킵니다. 루트로 동작하는 컨테이너 러너에서는 아래 테스트가 모두 실패합니다. 각 성공 경로에서 비루트 신원을 고정하십시오.
reviewer/tests/test_patch_validation.py#L419-L456:monkeypatch.setattr(patch_validation.os, "getuid", lambda: 1000)과getgid스텁을 추가하십시오. 같은 파일의 Line 693-717에도 적용하십시오.reviewer/tests/test_patch_validation_security_boundaries.py#L307-L345: 동일한 UID/GID 스텁을 추가하십시오. Line 401-418에도 적용하십시오.reviewer/tests/test_patch_validation_git_control_isolation.py#L91-L150: 동일한 UID/GID 스텁을 추가하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/tests/test_patch_validation.py` around lines 419 - 456, Fix the successful validation tests so they do not depend on the process identity by stubbing patch_validation.os.getuid and getgid to return 1000 before invoking DockerPatchValidationRunner.validate. Apply this to reviewer/tests/test_patch_validation.py lines 419-456 and 693-717, reviewer/tests/test_patch_validation_security_boundaries.py lines 307-345 and 401-418, and reviewer/tests/test_patch_validation_git_control_isolation.py lines 91-150; each listed site requires the same UID/GID stubs.reviewer/tests/test_patch_validation_hardening.py (1)
44-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win픽스처가 부분 경로
"git"을 사용합니다.patch_validation.TRUSTED_GIT_EXECUTABLE로 통일하십시오. 두 파일의 저장소 픽스처는"git"을 실행합니다. 같은 cohort의 다른 테스트는 절대 경로 상수를 사용하며, Ruff는 이 두 파일에서만 S607을 보고합니다.
reviewer/tests/test_patch_validation_hardening.py#L44-L71:_git_repository의 모든subprocess.run호출에서"git"을patch_validation.TRUSTED_GIT_EXECUTABLE로 바꾸십시오.reviewer/tests/test_patch_validation_git_metadata_mask.py#L40-L77:_initialize_repository와 Line 133-136의worktree add호출에서 같은 상수를 사용하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reviewer/tests/test_patch_validation_hardening.py` around lines 44 - 71, Replace the partial-path "git" executable in every subprocess.run call within _git_repository in reviewer/tests/test_patch_validation_hardening.py:44-71 with patch_validation.TRUSTED_GIT_EXECUTABLE. Apply the same replacement in _initialize_repository and the worktree add invocation at reviewer/tests/test_patch_validation_git_metadata_mask.py:40-77, preserving all other command arguments.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reviewer/noema_reviewer/patch_validation.py`:
- Around line 849-861: Update _git_metadata_kind to catch OSError from os.lstat
after preserving the existing FileNotFoundError handling, then wrap the
remaining filesystem error in RuntimeError so callers consistently receive the
module’s normalized exception type.
In `@reviewer/tests/test_patch_validation_canonical_paths.py`:
- Around line 31-39: Update inspect_patch_bytes to accept unquoted paths
containing spaces in diff --git headers, as well as matching unquoted --- and
+++ headers, instead of treating the resulting extra tokens as malformed. Add a
regression test alongside test_canonical_path_with_spaces_remains_supported
using unquoted src/file name.ts and assert the canonical result is ("src/file
name.ts",).
In `@reviewer/tests/test_patch_validation_prearchive_and_result_channel.py`:
- Around line 99-128: Update fake_run and the test around
_materialize_committed_source to mock the subprocess.Popen streaming path used
by _verify_exact_tree_limits, including the ls-tree output and required process
behavior. Replace the broad RuntimeError regex with a direct assertion for the
oversized-entry/byte-limit validation message, while retaining the assertion
that archive execution never starts.
- Line 58: pytest.raises 호출의 match 인자를 일반 문자열이 아닌 raw 문자열로 변경하여 RUF043 경고를
제거하십시오. test_patch_validation_prearchive_and_result_channel.py의 해당 테스트에서
“result.*must not be empty” 패턴과 동일하게 지적된 세 위치(58, 70, 120행)의 정규식 패턴에 적용하고, 기존 매칭
동작은 유지하십시오.
Apply the same fix in
`@reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py` at line 180:
The same raw-regex-literal remediation applies to this assertion.
In `@reviewer/tests/test_patch_validation_source_integrity.py`:
- Around line 155-208: Use stage-specific exception match patterns in
test_snapshot_materialization_rejects_failed_archive and
test_snapshot_materialization_rejects_invalid_archive so they cannot match the
preflight “source commit snapshot could not be materialized safely” message;
also track archive invocation in each subprocess double and assert it occurred,
while preserving the existing cleanup assertion for the invalid archive case.
---
Nitpick comments:
In `@reviewer/noema_reviewer/patch_validation.py`:
- Around line 1402-1415: Remove the unused _completed parameter from
_read_result_payload and update its callers, including the call that passes
completed, so the function signature and invocation no longer accept or provide
this argument.
- Around line 1455-1458: Remove the unreachable metadata_kind is None check
after _verify_source_head in the source validation flow, since
_verify_source_head already raises for that case. If the check is required
solely for type narrowing, replace it with an explanatory comment; otherwise
leave the existing _verify_source_head call and subsequent logic unchanged.
- Around line 1016-1038: Update _validated_exact_tree_output to reuse
_consume_exact_tree_stream rather than independently splitting records and
invoking _validated_exact_tree_record. Preserve the existing validation limits
and rules while returning or exposing the validated inventory needed by tests,
so the test helper does not maintain a duplicate implementation.
- Around line 1197-1203: Optimize the empty-directory validation loop in the
declared_directories check by building a set of parent directory paths from
declared_paths once, then use constant-time membership checks for each directory
instead of repeatedly scanning declared_paths. Preserve the existing path-prefix
semantics and ValueError behavior for empty gitlink-like directories.
In `@reviewer/tests/test_patch_validation_hardening.py`:
- Around line 44-71: Replace the partial-path "git" executable in every
subprocess.run call within _git_repository in
reviewer/tests/test_patch_validation_hardening.py:44-71 with
patch_validation.TRUSTED_GIT_EXECUTABLE. Apply the same replacement in
_initialize_repository and the worktree add invocation at
reviewer/tests/test_patch_validation_git_metadata_mask.py:40-77, preserving all
other command arguments.
In `@reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py`:
- Around line 51-78: Update the _git_repository helper to run every Git command
through the established patch_validation.TRUSTED_GIT_EXECUTABLE-based _run_git
pattern, preserving shell=False and timeout=30 for each invocation. Replace
direct "git" subprocess calls and reuse the existing shared helper conventions
used by the related patch-validation tests.
In `@reviewer/tests/test_patch_validation_streaming_edges.py`:
- Around line 413-433: Update
test_consume_exact_tree_stream_accepts_partial_records to capture the return
value from _consume_exact_tree_stream and assert the reconstructed inventory’s
path, mode, object ID, and size, ensuring fields split across chunk boundaries
are assembled correctly.
- Around line 436-451: Update
test_consume_exact_tree_stream_rejects_unterminated_record_ceiling so the
_read_git_stream_chunk monkeypatch returns data from a finite iterator rather
than always returning b"12345". Keep the first returned chunk sufficient to
trigger the record byte-limit error, and ensure subsequent reads are exhausted
so a regression cannot cause the test to hang.
In `@reviewer/tests/test_patch_validation_streaming_final_edges.py`:
- Around line 25-29: Replace the global subprocess.run monkeypatch in the
affected tests with a narrowly scoped stub for the call used by
_verify_source_head. Ensure only that validation path receives the fake return
value, while unrelated subprocess.run calls retain their real behavior; apply
the same change to both highlighted test locations.
In `@reviewer/tests/test_patch_validation.py`:
- Around line 419-456: Fix the successful validation tests so they do not depend
on the process identity by stubbing patch_validation.os.getuid and getgid to
return 1000 before invoking DockerPatchValidationRunner.validate. Apply this to
reviewer/tests/test_patch_validation.py lines 419-456 and 693-717,
reviewer/tests/test_patch_validation_security_boundaries.py lines 307-345 and
401-418, and reviewer/tests/test_patch_validation_git_control_isolation.py lines
91-150; each listed site requires the same UID/GID stubs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b0212ff-2c99-4ab7-a7ce-3d930061f5f5
📒 Files selected for processing (27)
CHANGELOG.mddocs/doctoring/quarantined-patch-validation.mddocs/quarantined-patch-validation.mdreviewer/noema_reviewer/__init__.pyreviewer/noema_reviewer/patch_validation.pyreviewer/tests/test_patch_validation.pyreviewer/tests/test_patch_validation_archive_boundaries.pyreviewer/tests/test_patch_validation_blob_identity_edges.pyreviewer/tests/test_patch_validation_canonical_paths.pyreviewer/tests/test_patch_validation_coverage_edges.pyreviewer/tests/test_patch_validation_exact_tree_and_output.pyreviewer/tests/test_patch_validation_exact_tree_archive_binding.pyreviewer/tests/test_patch_validation_exact_tree_canonical_metadata.pyreviewer/tests/test_patch_validation_git_control_descriptor_edge.pyreviewer/tests/test_patch_validation_git_control_isolation.pyreviewer/tests/test_patch_validation_git_metadata_mask.pyreviewer/tests/test_patch_validation_hardening.pyreviewer/tests/test_patch_validation_mode_boundaries.pyreviewer/tests/test_patch_validation_object_alternates_boundary.pyreviewer/tests/test_patch_validation_path_consistency.pyreviewer/tests/test_patch_validation_prearchive_and_result_channel.pyreviewer/tests/test_patch_validation_provenance_and_hunk_edges.pyreviewer/tests/test_patch_validation_security_boundaries.pyreviewer/tests/test_patch_validation_source_integrity.pyreviewer/tests/test_patch_validation_streaming_edges.pyreviewer/tests/test_patch_validation_streaming_final_edges.pyreviewer/tests/test_patch_validation_streaming_git_output.py
Comment on lines
+849
to
+861
| def _git_metadata_kind(source: Path) -> GitMetadataKind | None: | ||
| """Return safe Git-control metadata shape or reject special-file redirection.""" | ||
| try: | ||
| metadata = os.lstat(source / ".git") | ||
| except FileNotFoundError: | ||
| return None | ||
| if stat.S_ISLNK(metadata.st_mode) or not ( | ||
| stat.S_ISDIR(metadata.st_mode) or stat.S_ISREG(metadata.st_mode) | ||
| ): | ||
| raise RuntimeError( | ||
| "source Git metadata must not be a symlink and must be a regular file or directory" | ||
| ) | ||
| return "directory" if stat.S_ISDIR(metadata.st_mode) else "file" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
_git_metadata_kind는 FileNotFoundError 외의 OSError를 그대로 전파합니다.
예를 들어 상위 디렉터리 권한이 없으면 PermissionError가 호출자까지 올라갑니다. 이 모듈의 다른 실패는 모두 RuntimeError로 정규화됩니다. 호출자가 RuntimeError만 처리한다고 가정하면 예외 유형이 어긋납니다. OSError를 RuntimeError로 감싸십시오.
🛡️ 제안 수정
try:
metadata = os.lstat(source / ".git")
except FileNotFoundError:
return None
+ except OSError as exc:
+ raise RuntimeError("source Git metadata is unavailable") from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _git_metadata_kind(source: Path) -> GitMetadataKind | None: | |
| """Return safe Git-control metadata shape or reject special-file redirection.""" | |
| try: | |
| metadata = os.lstat(source / ".git") | |
| except FileNotFoundError: | |
| return None | |
| if stat.S_ISLNK(metadata.st_mode) or not ( | |
| stat.S_ISDIR(metadata.st_mode) or stat.S_ISREG(metadata.st_mode) | |
| ): | |
| raise RuntimeError( | |
| "source Git metadata must not be a symlink and must be a regular file or directory" | |
| ) | |
| return "directory" if stat.S_ISDIR(metadata.st_mode) else "file" | |
| def _git_metadata_kind(source: Path) -> GitMetadataKind | None: | |
| """Return safe Git-control metadata shape or reject special-file redirection.""" | |
| try: | |
| metadata = os.lstat(source / ".git") | |
| except FileNotFoundError: | |
| return None | |
| except OSError as exc: | |
| raise RuntimeError("source Git metadata is unavailable") from exc | |
| if stat.S_ISLNK(metadata.st_mode) or not ( | |
| stat.S_ISDIR(metadata.st_mode) or stat.S_ISREG(metadata.st_mode) | |
| ): | |
| raise RuntimeError( | |
| "source Git metadata must not be a symlink and must be a regular file or directory" | |
| ) | |
| return "directory" if stat.S_ISDIR(metadata.st_mode) else "file" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@reviewer/noema_reviewer/patch_validation.py` around lines 849 - 861, Update
_git_metadata_kind to catch OSError from os.lstat after preserving the existing
FileNotFoundError handling, then wrap the remaining filesystem error in
RuntimeError so callers consistently receive the module’s normalized exception
type.
Comment on lines
+31
to
+39
| def test_canonical_path_with_spaces_remains_supported() -> None: | ||
| """An exact quoted path with ordinary spaces remains a valid identity.""" | ||
| patch_bytes = ( | ||
| b'diff --git "a/src/file name.ts" "b/src/file name.ts"\n' | ||
| b'--- "a/src/file name.ts"\n' | ||
| b'+++ "b/src/file name.ts"\n' | ||
| ) | ||
|
|
||
| assert inspect_patch_bytes(patch_bytes) == ("src/file name.ts",) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 비인용 공백 경로 처리 경로와 기존 테스트 커버리지를 확인한다.
fd -t f 'patch_validation.py' reviewer --exec rg -n -C 6 'shlex.split|malformed diff header'
rg -n 'file name|unquoted' reviewer/testsRepository: ContextualWisdomLab/noema
Length of output: 3774
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- AGENTS.md files ---'
find .. -name AGENTS.md -print
for f in $(find .. -name AGENTS.md -print); do
printf '\n--- %s ---\n' "$f"
cat "$f"
done
printf '%s\n' '--- patch validator structure ---'
ast-grep outline reviewer/noema_reviewer/patch_validation.py
printf '%s\n' '--- relevant validator code ---'
sed -n '560,620p' reviewer/noema_reviewer/patch_validation.py
sed -n '700,750p' reviewer/noema_reviewer/patch_validation.py
printf '%s\n' '--- relevant tests ---'
sed -n '1,70p' reviewer/tests/test_patch_validation_canonical_paths.py
sed -n '165,215p' reviewer/tests/test_patch_validation.py
sed -n '175,210p' reviewer/tests/test_patch_validation_security_boundaries.pyRepository: ContextualWisdomLab/noema
Length of output: 18950
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import shlex
cases = [
"diff --git a/src/file name.ts b/src/file name.ts",
'diff --git "a/src/file name.ts" "b/src/file name.ts"',
]
for line in cases:
parts = shlex.split(line)
print(f"{line!r}")
print(f" tokens={len(parts)}: {parts!r}")
source = open("reviewer/noema_reviewer/patch_validation.py", encoding="utf-8").read()
needle = "if len(parts) != 4 or parts[:2] != [\"diff\", \"--git\"]:"
print(f"validator_requires_exactly_four_tokens={needle in source}")
print("existing_unquoted_space_tests:")
for path in (
"reviewer/tests/test_patch_validation_canonical_paths.py",
"reviewer/tests/test_patch_validation.py",
"reviewer/tests/test_patch_validation_security_boundaries.py",
):
text = open(path, encoding="utf-8").read()
print(f" {path}: {sum('diff --git a/src/file name.ts b/src/file name.ts' in line for line in text.splitlines())}")
PY
printf '%s\n' '--- repository references to unquoted Git headers ---'
rg -n -i 'unquoted|diff --git [^"]* [^"]*' reviewer README.md .github 2>/dev/null || trueRepository: ContextualWisdomLab/noema
Length of output: 9372
🌐 Web query:
Git diff --git header unquoted file paths containing spaces official documentation
💡 Result:
In the default output format of git diff, file paths in the diff header (the line starting with diff --git) and extended headers are not inherently "quoted" in the traditional sense, even if they contain spaces [1][2]. Instead, Git handles pathnames with "unusual" characters—including spaces, tabs, and non-printable characters—by applying C-style escaping if the core.quotePath configuration variable is enabled (which it is by default) [1][2][3]. When core.quotePath is true (default), paths containing such characters are enclosed in double quotes, and the special characters within them are escaped (e.g., spaces may be represented literally within the quotes, but characters like backslashes, tabs, or non-ASCII characters will be escaped) [1][3][4]. If you require raw, machine-readable output that avoids this munging of pathnames, you should use the -z option (e.g., git diff -z) with commands like --raw, --name-only, or --name-status [5][6]. The -z option forces Git to output the pathname verbatim, using NUL bytes as terminators instead of newlines, which safely handles any characters within the filename [3][6][4].
Citations:
- 1: https://git-scm.com/docs/diff-format
- 2: https://git-scm.com/docs/git-diff
- 3: https://git-scm.com/docs/diff-options/2.52.0
- 4: https://cdn.kernel.org/pub/software/scm/git/docs/git-diff.html
- 5: https://stackoverflow.com/questions/28109520/how-to-cope-with-spaces-in-file-names-when-iterating-results-from-git-diff-nam
- 6: https://git-scm.com/docs/git-diff/2.45.3
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Git version and read-only path quoting probes ---'
git --version
set +e
git diff --no-index \
--label='a/src/file name.ts' \
--label='b/src/file name.ts' \
/etc/hosts /etc/passwd > /tmp/noema-git-label-diff.txt
status=$?
set -e
printf 'exit_status=%s\n' "$status"
sed -n '1,8p' /tmp/noema-git-label-diff.txt
set +e
git diff --no-index \
--src-prefix='a/' \
--dst-prefix='b/' \
'/etc/hosts' '/etc/passwd' > /tmp/noema-git-prefix-diff.txt
status=$?
set -e
printf 'exit_status=%s\n' "$status"
sed -n '1,8p' /tmp/noema-git-prefix-diff.txt
printf '%s\n' '--- Official Git diff-format wording ---'
curl -L --fail --silent --show-error https://git-scm.com/docs/diff-format \
| rg -n -i -C 3 'diff --git|quote|unusual|space' | head -80Repository: ContextualWisdomLab/noema
Length of output: 10576
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Read-only Git quoting probe with spaces in generated prefixes ---'
set +e
git -c core.quotePath=true diff --no-index \
--src-prefix='a/src/file name.ts/' \
--dst-prefix='b/src/file name.ts/' \
/etc/hosts /etc/passwd > /tmp/noema-git-space-prefix-diff.txt
status=$?
set -e
printf 'exit_status=%s\n' "$status"
sed -n '1,5p' /tmp/noema-git-space-prefix-diff.txt
printf '%s\n' '--- Git quotePath documentation ---'
curl -L --fail --silent --show-error https://git-scm.com/docs/git-config \
| rg -n -i -C 5 'core\.quotePath|quote.*path' | head -60
printf '%s\n' '--- Git quote implementation references ---'
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/git/git/v2.50.1/quote.c \
| rg -n -C 8 'quote_c_style|needquote|isspace|0x20|32'Repository: ContextualWisdomLab/noema
Length of output: 6109
비인용 공백 경로를 지원하고 회귀 테스트를 추가하십시오.
Git은 diff --git a/src/file name.ts b/src/file name.ts처럼 공백을 포함한 경로를 인용하지 않을 수 있습니다. 현재 shlex.split(line)은 6개 토큰을 반환하므로 inspect_patch_bytes가 "patch contains a malformed diff header"로 거부합니다. 동일한 비인용 경로를 --- 및 +++ 헤더에도 사용하고, 반환값이 ("src/file name.ts",)인지 테스트하십시오.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@reviewer/tests/test_patch_validation_canonical_paths.py` around lines 31 -
39, Update inspect_patch_bytes to accept unquoted paths containing spaces in
diff --git headers, as well as matching unquoted --- and +++ headers, instead of
treating the resulting extra tokens as malformed. Add a regression test
alongside test_canonical_path_with_spaces_remains_supported using unquoted
src/file name.ts and assert the canonical result is ("src/file name.ts",).
| result_path.touch(mode=0o600) | ||
| completed = SimpleNamespace(returncode=0, stdout='{"status":"passed"}') | ||
|
|
||
| with pytest.raises(RuntimeError, match="result.*must not be empty"): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use raw-string regex literals for pytest.raises(..., match=...) patterns containing regular-expression metacharacters. This applies to the patterns at lines 58, 70, 120 in this file and the hunk|trailing|syntax pattern in reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py:180 so Ruff RUF043 is not triggered.
📍 Affects 2 files
reviewer/tests/test_patch_validation_prearchive_and_result_channel.py#L58-L58(this comment)reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py#L180-L180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@reviewer/tests/test_patch_validation_prearchive_and_result_channel.py` at
line 58, pytest.raises 호출의 match 인자를 일반 문자열이 아닌 raw 문자열로 변경하여 RUF043 경고를 제거하십시오.
test_patch_validation_prearchive_and_result_channel.py의 해당 테스트에서 “result.*must
not be empty” 패턴과 동일하게 지적된 세 위치(58, 70, 120행)의 정규식 패턴에 적용하고, 기존 매칭 동작은 유지하십시오.
Apply the same fix in
`@reviewer/tests/test_patch_validation_provenance_and_hunk_edges.py` at line 180:
The same raw-regex-literal remediation applies to this assertion.
Source: Linters/SAST tools
Comment on lines
+99
to
+128
| def fake_run(command, **_kwargs): | ||
| """Expose an oversized exact-tree entry and forbid archive execution.""" | ||
| nonlocal archive_started | ||
| command_list = list(command) | ||
| if "ls-tree" in command_list: | ||
| return SimpleNamespace( | ||
| returncode=0, | ||
| stdout=( | ||
| "100644 blob " | ||
| f"{'a' * 40} " | ||
| f"{patch_validation.MAX_SOURCE_ARCHIVE_MEMBER_BYTES + 1}" | ||
| "\toversized.bin\0" | ||
| ), | ||
| ) | ||
| if "archive" in command_list: | ||
| archive_started = True | ||
| return SimpleNamespace(returncode=0) | ||
| raise AssertionError(f"unexpected Git command: {command_list}") | ||
|
|
||
| monkeypatch.setattr(patch_validation.subprocess, "run", fake_run) | ||
|
|
||
| with pytest.raises(RuntimeError, match="tree|byte limit|materialized"): | ||
| patch_validation._materialize_committed_source( | ||
| source, | ||
| "1" * 40, | ||
| staging, | ||
| "directory", | ||
| ) | ||
|
|
||
| assert archive_started is False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check which subprocess API the exact-tree preflight uses.
set -euo pipefail
fd -t f 'patch_validation.py' reviewer --exec ast-grep run --lang python --pattern 'def _verify_exact_tree_limits($$$):
$$$'
rg -nP -C3 'subprocess\.(run|Popen)' reviewer/noema_reviewer/patch_validation.pyRepository: ContextualWisdomLab/noema
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AGENTS.md files ---'
find .. -name AGENTS.md -print
printf '%s\n' '--- candidate files ---'
fd -t f | rg '(^|/)(patch_validation\.py|test_patch_validation_prearchive_and_result_channel\.py)$|^docs/quarantined-patch-validation\.md$'
printf '%s\n' '--- subprocess usage ---'
rg -n -C5 'subprocess\.(run|Popen)|_verify_exact_tree_limits|_materialize_committed_source' reviewer docs 2>/dev/null || trueRepository: ContextualWisdomLab/noema
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AGENTS.md ---'
cat -n AGENTS.md
printf '%s\n' '--- exact-tree implementation ---'
sed -n '430,490p;1030,1115p;1307,1355p' reviewer/noema_reviewer/patch_validation.py
printf '%s\n' '--- reviewed test ---'
sed -n '1,155p' reviewer/tests/test_patch_validation_prearchive_and_result_channel.py
printf '%s\n' '--- related streaming tests ---'
sed -n '1,120p' reviewer/tests/test_patch_validation_streaming_final_edges.py
sed -n '1,115p' reviewer/tests/test_patch_validation_streaming_git_output.py
printf '%s\n' '--- documentation ---'
sed -n '35,60p' docs/quarantined-patch-validation.mdRepository: ContextualWisdomLab/noema
Length of output: 25986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
implementation = Path("reviewer/noema_reviewer/patch_validation.py")
test_file = Path("reviewer/tests/test_patch_validation_prearchive_and_result_channel.py")
impl_tree = ast.parse(implementation.read_text())
test_tree = ast.parse(test_file.read_text())
def function(tree, name):
return next(node for node in ast.walk(tree)
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
and node.name == name)
verify = function(impl_tree, "_verify_exact_tree_limits")
materialize = function(impl_tree, "_materialize_committed_source")
test = function(test_tree, "test_exact_tree_limits_are_checked_before_git_archive")
verify_calls = [
ast.unparse(node)
for node in ast.walk(verify)
if isinstance(node, ast.Call)
]
materialize_calls = [
ast.unparse(node)
for node in ast.walk(materialize)
if isinstance(node, ast.Call)
]
patched_targets = [
ast.unparse(node)
for node in ast.walk(test)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "setattr"
]
print("verify subprocess APIs:", [
call for call in verify_calls
if "subprocess." in call or "_start_git_stream" in call
])
print("materialize relevant calls:", [
call for call in materialize_calls
if "_verify_exact_tree_limits" in call or "subprocess.run" in call
])
print("test monkeypatch targets:", patched_targets)
assert any("_start_git_stream" in call for call in verify_calls)
assert not any("subprocess.run" in call for call in verify_calls)
assert any("patch_validation.subprocess, 'run'" in target
for target in patched_targets)
PYRepository: ContextualWisdomLab/noema
Length of output: 1024
_verify_exact_tree_limits의 스트리밍 실행을 테스트에서 대체하십시오. ls-tree는 subprocess.Popen을 통해 실행되므로 현재 subprocess.run의 분기는 호출되지 않습니다. 빈 control 디렉터리에서 발생한 일반적인 materialization 오류도 현재 정규식과 일치하여 테스트가 잘못 통과할 수 있습니다. 스트리밍 API를 대체하고, 초과 한계 메시지를 직접 검사하십시오.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 120-120: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@reviewer/tests/test_patch_validation_prearchive_and_result_channel.py` around
lines 99 - 128, Update fake_run and the test around
_materialize_committed_source to mock the subprocess.Popen streaming path used
by _verify_exact_tree_limits, including the ls-tree output and required process
behavior. Replace the broad RuntimeError regex with a direct assertion for the
oversized-entry/byte-limit validation message, while retaining the assertion
that archive execution never starts.
Comment on lines
+155
to
+208
| def failed_archive(command, **_kwargs): | ||
| """Pass exact-tree preflight but fail the subsequent archive command.""" | ||
| command_list = list(command) | ||
| if "ls-tree" in command_list: | ||
| return _bounded_tree_result() | ||
| if "archive" in command_list: | ||
| return SimpleNamespace(returncode=1) | ||
| raise AssertionError(f"unexpected Git command: {command_list}") | ||
|
|
||
| monkeypatch.setattr(patch_validation.subprocess, "run", failed_archive) | ||
|
|
||
| with pytest.raises(RuntimeError, match="snapshot could not be materialized"): | ||
| patch_validation._materialize_committed_source( | ||
| tmp_path, | ||
| "2" * 40, | ||
| staging, | ||
| "directory", | ||
| ) | ||
|
|
||
|
|
||
| def test_snapshot_materialization_rejects_invalid_archive( | ||
| tmp_path: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Malformed archive bytes fail closed and the transient archive is removed.""" | ||
| staging = tmp_path / "staging" | ||
| staging.mkdir() | ||
| _isolated_control(staging, monkeypatch) | ||
|
|
||
| def corrupt_archive(command, **_kwargs): | ||
| """Pass preflight, then write invalid bytes at Git's archive output path.""" | ||
| command_list = list(command) | ||
| if "ls-tree" in command_list: | ||
| return _bounded_tree_result() | ||
| if "archive" not in command_list: | ||
| raise AssertionError(f"unexpected Git command: {command_list}") | ||
| output = next( | ||
| argument.removeprefix("--output=") | ||
| for argument in command_list | ||
| if argument.startswith("--output=") | ||
| ) | ||
| Path(output).write_bytes(b"not a tar archive") | ||
| return SimpleNamespace(returncode=0) | ||
|
|
||
| monkeypatch.setattr(patch_validation.subprocess, "run", corrupt_archive) | ||
|
|
||
| with pytest.raises(RuntimeError, match="materialized safely"): | ||
| patch_validation._materialize_committed_source( | ||
| tmp_path, | ||
| "2" * 40, | ||
| staging, | ||
| "directory", | ||
| ) | ||
| assert not (staging / "source.tar").exists() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
match 패턴이 넓어 archive 단계 검증이 보증되지 않습니다.
Line 166의 패턴 "snapshot could not be materialized"는 preflight 실패 메시지 "source commit snapshot could not be materialized safely"의 부분 문자열입니다. Line 201의 "materialized safely"도 같은 preflight 실패와 일치합니다. ls-tree 더블이 호출되지 않으면 두 테스트는 archive 반환 코드 경로나 tar 파싱 경로를 검증하지 않고도 통과합니다.
패턴을 각 단계의 고유 메시지에 고정하고, 각 더블이 실제로 호출되었는지 assert하십시오. 예로 failed_archive에 호출 플래그를 두고 assert archive_invoked is True를 추가하십시오.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@reviewer/tests/test_patch_validation_source_integrity.py` around lines 155 -
208, Use stage-specific exception match patterns in
test_snapshot_materialization_rejects_failed_archive and
test_snapshot_materialization_rejects_invalid_archive so they cannot match the
preflight “source commit snapshot could not be materialized safely” message;
also track archive invocation in each subprocess double and assert it occurred,
while preserving the existing cleanup assertion for the invalid archive case.
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Clean protected-main successor for closed stale PR #65 after #76 integrated. This PR starts from current protected
mainc85d710804139c0697d7ef8fa47d02b1389e6d84and carries the unique credential-free exact-source patch-quarantine implementation, tests, doctoring, and one current-baseline## Unreleasedchangelog entry without replaying #65's stale pre-#76 package/audit state.Current exact source boundary
feat/quarantined-patch-validation-on-main;fe3074a188739705f89ff67b46712ac9d228081d;main:c85d710804139c0697d7ef8fa47d02b1389e6d84;CHANGELOG.mdentry;PR #65 is already closed unmerged; its checks/reviews are historical and do not transfer.
Preserved product boundary
Fresh exact-head technical evidence
For unchanged exact head
fe3074a188739705f89ff67b46712ac9d228081d:cirun31375580944: terminal success;reviewer-cirun31375580959: terminal success;Security Scanrun31375580983: terminal success under its own scanner/revision semantics;These runs are fresh post-#76 technical evidence. They do not create live ruleset evidence or qualifying independent approval.
Current acceptance sequence
main.No repair/self-modifying workflow, force push, audit waiver, synthetic approval, version bump, release or license decision is introduced.
Related: #9, #27, #29, #65, #66, #67
Summary by CodeRabbit
새 기능
보안 강화
검증