fix(io): attempt atomic publication of generated index files - #296
fix(io): attempt atomic publication of generated index files#296seonghobae wants to merge 2 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthrough
Changes원자적 index.html 쓰기
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/kotlin/html4tree/MainTest.kt (1)
365-375: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFallback 테스트에서 기존 대상 교체를 검증하세요.
현재 Line 367에서 기존
index.htmldirectory를 삭제한 뒤 fallback을 실행합니다. 따라서 대상이 없는 경우만 테스트합니다.REPLACE_EXISTING이 기존 파일을 실제로 교체하는지 검증하지 않습니다. (docs.oracle.com)기존 일반 파일을 먼저 생성한 뒤
AtomicMoveNotSupportedException을 주입하세요. 새 내용으로 교체되는지 확인하세요. 가능하면 심볼릭 링크 대상이 변경되지 않는지도 fallback 경로에서 확인하세요.As per coding guidelines, any new Kotlin code or branch must have covering tests because JaCoCo enforces 100% coverage through
check.🤖 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 `@src/test/kotlin/html4tree/MainTest.kt` around lines 365 - 375, The fallback test around write_index_file must verify replacement of an existing index.html file, not only creation when the target is absent. Replace the indexDir.deleteRecursively setup with creation of an existing regular index.html containing different content, inject AtomicMoveNotSupportedException through mockMoveAtomic, and assert fallbackCalled plus the file’s updated content; if the test already supports symlinks, also confirm the symlink target remains unchanged.Source: Coding guidelines
.jules/sentinel.md (1)
86-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFallback의 비원자성 및 보장 범위를 문서에 명시하세요.
REPLACE_EXISTINGfallback은ATOMIC_MOVE보장을 유지하지 않습니다.ATOMIC_MOVE없이 수행하는 이동은 대상 확인과 실제 이동이 다른 파일시스템 작업과 원자적이지 않을 수 있습니다. (docs.oracle.com)현재 제목과
Prevention문구는 fallback에서도 TOCTOU와 경쟁 상태가 방지되는 것처럼 읽힐 수 있습니다. 원자 이동은 지원되는 파일시스템에서만 보장되며 fallback은 호환성을 위한 비원자 경로라는 점을 명시하세요. 보안 보장이 필수이면 fallback 대신 실패하도록 구현과 문서를 맞추세요.🤖 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 @.jules/sentinel.md around lines 86 - 90, Update the html4tree atomic file move documentation to state that ATOMIC_MOVE provides the atomicity guarantee only on supporting filesystems, while the REPLACE_EXISTING fallback is non-atomic and retained solely for compatibility. Clarify in the Prevention guidance that security-critical workflows should fail when atomic movement is unavailable, and align the documented implementation behavior with that requirement.
🤖 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 `@src/main/kotlin/html4tree/main.kt`:
- Around line 233-248: Update write_index_file in
src/main/kotlin/html4tree/main.kt:233-248 to define and document the replacement
policy, explicitly handle replacement failures beyond
AtomicMoveNotSupportedException, and either reject or deliberately use the
non-atomic fallback while preserving cleanup. In
src/test/kotlin/html4tree/MainTest.kt:365-375, add coverage that keeps an
existing index.html and verifies fallback replacement. In
.jules/sentinel.md:86-90, state that the fallback is a compatibility path and
does not guarantee atomicity.
---
Nitpick comments:
In @.jules/sentinel.md:
- Around line 86-90: Update the html4tree atomic file move documentation to
state that ATOMIC_MOVE provides the atomicity guarantee only on supporting
filesystems, while the REPLACE_EXISTING fallback is non-atomic and retained
solely for compatibility. Clarify in the Prevention guidance that
security-critical workflows should fail when atomic movement is unavailable, and
align the documented implementation behavior with that requirement.
In `@src/test/kotlin/html4tree/MainTest.kt`:
- Around line 365-375: The fallback test around write_index_file must verify
replacement of an existing index.html file, not only creation when the target is
absent. Replace the indexDir.deleteRecursively setup with creation of an
existing regular index.html containing different content, inject
AtomicMoveNotSupportedException through mockMoveAtomic, and assert
fallbackCalled plus the file’s updated content; if the test already supports
symlinks, also confirm the symlink target remains unchanged.
🪄 Autofix (Beta)
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: f25aee6a-84ac-4cb9-bf9a-3399bf0f6d5d
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/main/kotlin/html4tree/main.ktsrc/test/kotlin/html4tree/MainTest.kt
| fun write_index_file( | ||
| curr_dir: File, | ||
| content: String, | ||
| moveAtomic: (java.nio.file.Path, java.nio.file.Path) -> Unit = { source, target -> | ||
| Files.move(source, target, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING) | ||
| } | ||
| ) { | ||
| val indexPath = curr_dir.toPath().resolve("index.html") | ||
| val tempPath = Files.createTempFile(curr_dir.toPath(), ".index-", ".html") | ||
| try { | ||
| Files.write(tempPath, content.toByteArray(Charsets.UTF_8)) | ||
| Files.move(tempPath, indexPath, StandardCopyOption.REPLACE_EXISTING) | ||
| try { | ||
| moveAtomic(tempPath, indexPath) | ||
| } catch (e: AtomicMoveNotSupportedException) { | ||
| Files.move(tempPath, indexPath, StandardCopyOption.REPLACE_EXISTING) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target implementation ---'
sed -n '210,270p' src/main/kotlin/html4tree/main.kt
printf '%s\n' '--- related test ---'
sed -n '335,395p' src/test/kotlin/html4tree/MainTest.kt
printf '%s\n' '--- sentinel entry ---'
sed -n '75,100p' .jules/sentinel.md
printf '%s\n' '--- relevant usages and declarations ---'
rg -n -C 3 'write_index_file|AtomicMoveNotSupportedException|ATOMIC_MOVE|index\.html' src/main src/test .julesRepository: ContextualWisdomLab/html4tree
Length of output: 27307
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
for p in [
Path("src/main/kotlin/html4tree/main.kt"),
Path("src/test/kotlin/html4tree/MainTest.kt"),
]:
text = p.read_text()
print(f"{p}: {len(text.splitlines())} lines")
for i, line in enumerate(text.splitlines(), 1):
if "write_index_file" in line or "AtomicMoveNotSupportedException" in line or "ATOMIC_MOVE" in line:
print(f"{i}: {line}")
PYRepository: ContextualWisdomLab/html4tree
Length of output: 1072
🌐 Web query:
Java Files.move ATOMIC_MOVE REPLACE_EXISTING existing target option ignored provider-specific replacement official documentation
💡 Result:
According to the official Java documentation for the java.nio.file.Files.move method, when the ATOMIC_MOVE option is specified, all other CopyOption arguments (including REPLACE_EXISTING) are ignored [1][2]. The behavior of ATOMIC_MOVE regarding an existing target is defined as implementation-specific: if the target file exists, the underlying file system implementation may either replace the existing file or fail by throwing an IOException [1][3]. Because ATOMIC_MOVE must be performed as an atomic file system operation, the method will throw an AtomicMoveNotSupportedException if the operation cannot be completed atomically—such as when the target location is on a different FileStore or is associated with a different file system provider [1][2]. In summary, you cannot rely on REPLACE_EXISTING to force a replacement when using ATOMIC_MOVE. If you require atomic behavior and the target file might exist, you must handle the potential for an IOException or implementation-dependent behavior [1][4].
Citations:
- 1: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/nio/file/Files.html
- 2: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/nio/file/Files.html
- 3: https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html
- 4: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/Files.html
🏁 Script executed:
#!/bin/bash
set -eu
# Probe the host JDK's default provider with an existing regular target.
# This standalone probe does not load or execute repository code.
if command -v java >/dev/null 2>&1; then
java -version 2>&1 | head -n 2
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/MoveProbe.java" <<'JAVA'
import java.nio.charset.StandardCharsets;
import java.nio.file.*;
public class MoveProbe {
public static void main(String[] args) throws Exception {
Path dir = Paths.get(args[0]);
Path source = Files.writeString(dir.resolve("source"), "new", StandardCharsets.UTF_8);
Path target = Files.writeString(dir.resolve("target"), "old", StandardCharsets.UTF_8);
try {
Files.move(source, target, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
System.out.println("atomic_move=" + Files.readString(target));
} catch (Exception e) {
System.out.println("atomic_move_exception=" + e.getClass().getName());
System.out.println("target_after_atomic=" + Files.readString(target));
}
Path fallbackSource = Files.writeString(dir.resolve("fallback-source"), "fallback", StandardCharsets.UTF_8);
try {
Files.move(fallbackSource, target, StandardCopyOption.REPLACE_EXISTING);
System.out.println("fallback_move=" + Files.readString(target));
} catch (Exception e) {
System.out.println("fallback_move_exception=" + e.getClass().getName());
}
}
}
JAVA
javac "$tmpdir/MoveProbe.java"
java -cp "$tmpdir" MoveProbe "$tmpdir"
else
echo "java_unavailable"
fiRepository: ContextualWisdomLab/html4tree
Length of output: 325
원자적 교체 정책을 코드, 테스트, 문서에서 일치시키세요.
ATOMIC_MOVE를 지정하면 REPLACE_EXISTING은 무시됩니다. 기존 index.html의 교체 여부는 파일 시스템 제공자에 따라 달라집니다. AtomicMoveNotSupportedException 이외의 교체 실패도 명시적으로 처리하고, 비원자적 fallback을 허용할지 결정하세요.
src/test/kotlin/html4tree/MainTest.kt:366-375: 기존index.html을 남긴 상태에서 fallback 교체를 테스트하세요..jules/sentinel.md:87-90: fallback이 원자성을 보장하지 않는 호환성 경로임을 명시하세요.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 246-246: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
📍 Affects 3 files
src/main/kotlin/html4tree/main.kt#L233-L248(this comment)src/test/kotlin/html4tree/MainTest.kt#L365-L375.jules/sentinel.md#L86-L90
🤖 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 `@src/main/kotlin/html4tree/main.kt` around lines 233 - 248, Update
write_index_file in src/main/kotlin/html4tree/main.kt:233-248 to define and
document the replacement policy, explicitly handle replacement failures beyond
AtomicMoveNotSupportedException, and either reject or deliberately use the
non-atomic fallback while preserving cleanup. In
src/test/kotlin/html4tree/MainTest.kt:365-375, add coverage that keeps an
existing index.html and verifies fallback replacement. In
.jules/sentinel.md:86-90, state that the fallback is a compatibility path and
does not guarantee atomicity.
Source: Coding guidelines
seonghobae
changed the title
1 participant
Reliability problem
write_index_filealready writes the complete UTF-8 document to a same-directory temporary file before replacingindex.html, but the finalFiles.moveuses onlyREPLACE_EXISTING. On filesystems that support atomic moves, a single atomic replacement gives concurrent readers a clearer all-old-or-all-new publication boundary.This is a reliability/availability hardening change. It is not evidence that every TOCTOU or symlink threat is eliminated, and the
REPLACE_EXISTINGfallback is explicitly non-atomic.Proposed bounded change
Files.move(..., ATOMIC_MOVE, REPLACE_EXISTING)through an injectable move boundary;AtomicMoveNotSupportedExceptionand use the existing non-atomic replacement fallback;Required before ready
This head is intentionally not merge-ready. It must first update to the protected
masterafter canonical CSP PR #363 integrates, preserve every current dependency/supply-chain/security control, and then add:CHANGELOG.mdand APA 7 doctoring grounded in current Oracle/JDKFiles.moveandStandardCopyOption.ATOMIC_MOVEdocumentation;Queued, pending, skipped-required, cancelled, absent, stale-head, or failed checks are not success. No review or check from duplicate atomic-move branches is reusable.