Skip to content

Commit dfd8ba6

Browse files
committed
fix: handle partial-read trailing newline when not at EOF
Add reachedEOF flag to readFileWithSmartPositioning return metadata. When a partial read does not reach EOF, the last line read definitely had a newline after it (more content follows), so always restore it. Only use the endsWithNewline heuristic when the read actually reached the end of the file. Fixes edge case identified in CodeRabbit review on PR wonderwhy-er#438.
1 parent 0486a76 commit dfd8ba6

3 files changed

Lines changed: 144 additions & 9 deletions

File tree

src/utils/files/base.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,9 @@ export interface FileMetadata {
126126
/** For text files */
127127
lineCount?: number;
128128

129+
/** Whether the read reached the end of the file */
130+
reachedEOF?: boolean;
131+
129132
/** For PDF files */
130133
isPdf?: boolean;
131134
author?: string;

src/utils/files/text.ts

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,15 @@ export class TextFileHandler implements FileHandler {
6969
result.content = normalizeLineEndings(result.content, lineEnding);
7070
}
7171

72-
// Restore trailing newline stripped by readline
73-
if (endsWithNewline && typeof result.content === 'string' && !result.content.endsWith(lineEnding)) {
74-
result.content += lineEnding;
72+
// Restore trailing newline stripped by readline.
73+
// When we haven't reached EOF, the last line we read definitely had
74+
// a newline after it (there's more content following in the file).
75+
// When we have reached EOF, only restore if the original file ended with one.
76+
if (typeof result.content === 'string' && !result.content.endsWith(lineEnding)) {
77+
const reachedEOF = result.metadata?.reachedEOF !== false;
78+
if (!reachedEOF || endsWithNewline) {
79+
result.content += lineEnding;
80+
}
7581
}
7682

7783
return result;
@@ -362,7 +368,7 @@ export class TextFileHandler implements FileHandler {
362368
? `${this.generateEnhancedStatusMessage(result.length, -n, fileTotalLines, true)}\n\n${result.join('\n')}`
363369
: result.join('\n');
364370

365-
return { content, mimeType, metadata: {} };
371+
return { content, mimeType, metadata: { reachedEOF: true } };
366372
} finally {
367373
await fd.close();
368374
}
@@ -409,7 +415,7 @@ export class TextFileHandler implements FileHandler {
409415
? `${this.generateEnhancedStatusMessage(result.length, -requestedLines, fileTotalLines, true)}\n\n${result.join('\n')}`
410416
: result.join('\n');
411417

412-
return { content, mimeType, metadata: {} };
418+
return { content, mimeType, metadata: { reachedEOF: true } };
413419
}
414420

415421
/**
@@ -430,12 +436,15 @@ export class TextFileHandler implements FileHandler {
430436

431437
const result: string[] = [];
432438
let lineNumber = 0;
439+
let reachedEOF = true;
433440

434441
for await (const line of rl) {
435442
if (lineNumber >= offset && result.length < length) {
436443
result.push(line);
444+
} else if (result.length >= length) {
445+
reachedEOF = false;
446+
break;
437447
}
438-
if (result.length >= length) break;
439448
lineNumber++;
440449
}
441450

@@ -444,10 +453,10 @@ export class TextFileHandler implements FileHandler {
444453
if (includeStatusMessage) {
445454
const statusMessage = this.generateEnhancedStatusMessage(result.length, offset, fileTotalLines, false);
446455
const content = `${statusMessage}\n\n${result.join('\n')}`;
447-
return { content, mimeType, metadata: {} };
456+
return { content, mimeType, metadata: { reachedEOF } };
448457
} else {
449458
const content = result.join('\n');
450-
return { content, mimeType, metadata: {} };
459+
return { content, mimeType, metadata: { reachedEOF } };
451460
}
452461
}
453462

@@ -500,6 +509,7 @@ export class TextFileHandler implements FileHandler {
500509

501510
const result: string[] = [];
502511
let firstLineSkipped = false;
512+
let reachedEOF = true;
503513

504514
for await (const line of rl2) {
505515
if (!firstLineSkipped && startPosition > 0) {
@@ -510,6 +520,7 @@ export class TextFileHandler implements FileHandler {
510520
if (result.length < length) {
511521
result.push(line);
512522
} else {
523+
reachedEOF = false;
513524
break;
514525
}
515526
}
@@ -520,7 +531,7 @@ export class TextFileHandler implements FileHandler {
520531
? `${this.generateEnhancedStatusMessage(result.length, offset, fileTotalLines, false)}\n\n${result.join('\n')}`
521532
: result.join('\n');
522533

523-
return { content, mimeType, metadata: {} };
534+
return { content, mimeType, metadata: { reachedEOF } };
524535
} finally {
525536
await fd.close();
526537
}

test/test-crlf-preservation.js

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,22 @@ const __dirname = path.dirname(__filename);
2222
const TEST_DIR = path.join(__dirname, 'test_crlf_preservation');
2323
const CRLF_FILE = path.join(TEST_DIR, 'crlf_file.txt');
2424
const LF_FILE = path.join(TEST_DIR, 'lf_file.txt');
25+
const CRLF_NO_TRAILING_FILE = path.join(TEST_DIR, 'crlf_no_trailing.txt');
26+
const LF_NO_TRAILING_FILE = path.join(TEST_DIR, 'lf_no_trailing.txt');
2527

2628
// The raw CRLF content (5 lines, each ending with \r\n)
2729
const CRLF_CONTENT = 'Line one\r\nLine two\r\nLine three\r\nLine four\r\nLine five\r\n';
2830
const LF_CONTENT = 'Line one\nLine two\nLine three\nLine four\nLine five\n';
31+
// Files without trailing newline (last line has no newline after it)
32+
const CRLF_NO_TRAILING_CONTENT = 'Line one\r\nLine two\r\nLine three\r\nLine four\r\nLine five';
33+
const LF_NO_TRAILING_CONTENT = 'Line one\nLine two\nLine three\nLine four\nLine five';
2934

3035
async function setup() {
3136
await fs.mkdir(TEST_DIR, { recursive: true });
3237
await fs.writeFile(CRLF_FILE, CRLF_CONTENT, 'utf8');
3338
await fs.writeFile(LF_FILE, LF_CONTENT, 'utf8');
39+
await fs.writeFile(CRLF_NO_TRAILING_FILE, CRLF_NO_TRAILING_CONTENT, 'utf8');
40+
await fs.writeFile(LF_NO_TRAILING_FILE, LF_NO_TRAILING_CONTENT, 'utf8');
3441

3542
const originalConfig = await configManager.getConfig();
3643
await configManager.setValue('allowedDirectories', [TEST_DIR]);
@@ -210,6 +217,116 @@ async function testReadWriteRoundtrip() {
210217
console.log(' PASS');
211218
}
212219

220+
/**
221+
* Test 6: Partial read (not reaching EOF) preserves CRLF on last line
222+
* Edge case from CodeRabbit review: when reading only a subset of lines,
223+
* the last line read still has content after it in the file, so its
224+
* trailing newline must be preserved.
225+
*/
226+
async function testPartialReadPreservesCRLF() {
227+
console.log('\n Test 6: partial read (not reaching EOF) preserves CRLF');
228+
229+
// Read only first 2 lines of the 5-line CRLF file
230+
const result = await handleReadFile({ path: CRLF_FILE, offset: 0, length: 2 });
231+
const text = result.content[0].text;
232+
233+
const contentStart = text.indexOf('Line one');
234+
const fileContent = text.substring(contentStart);
235+
236+
// Should contain CRLF, not bare LF
237+
assert.ok(
238+
fileContent.includes('\r\n'),
239+
'Partial read must preserve CRLF line endings'
240+
);
241+
assert.ok(
242+
!fileContent.match(/(?<!\r)\n/),
243+
'Partial read must not contain bare LF'
244+
);
245+
// The last line read ("Line two") should end with \r\n because
246+
// there are more lines after it in the file
247+
assert.ok(
248+
fileContent.endsWith('\r\n'),
249+
'Last line of partial read must end with CRLF (more content follows in file)'
250+
);
251+
console.log(' PASS');
252+
}
253+
254+
/**
255+
* Test 7: Partial read (not reaching EOF) of LF file preserves LF on last line
256+
*/
257+
async function testPartialReadPreservesLF() {
258+
console.log('\n Test 7: partial read (not reaching EOF) preserves LF');
259+
260+
const result = await handleReadFile({ path: LF_FILE, offset: 0, length: 2 });
261+
const text = result.content[0].text;
262+
263+
const contentStart = text.indexOf('Line one');
264+
const fileContent = text.substring(contentStart);
265+
266+
assert.ok(
267+
!fileContent.includes('\r'),
268+
'LF partial read must not contain CR'
269+
);
270+
assert.ok(
271+
fileContent.endsWith('\n'),
272+
'Last line of LF partial read must end with LF (more content follows in file)'
273+
);
274+
console.log(' PASS');
275+
}
276+
277+
/**
278+
* Test 8: Full read of file without trailing newline must NOT add one
279+
*/
280+
async function testNoTrailingNewlinePreserved() {
281+
console.log('\n Test 8: full read of file without trailing newline does not add one');
282+
283+
// Read all 5 lines (the file has no trailing newline)
284+
const result = await handleReadFile({ path: CRLF_NO_TRAILING_FILE, offset: 0, length: 1000 });
285+
const text = result.content[0].text;
286+
287+
const contentStart = text.indexOf('Line one');
288+
const fileContent = text.substring(contentStart);
289+
290+
// All internal line endings should be CRLF
291+
assert.ok(
292+
fileContent.includes('\r\n'),
293+
'Internal line endings must be CRLF'
294+
);
295+
// But the content must NOT end with \r\n since the file doesn't
296+
assert.ok(
297+
!fileContent.endsWith('\r\n'),
298+
'Must not add trailing CRLF when original file lacks one'
299+
);
300+
assert.ok(
301+
fileContent.endsWith('Line five'),
302+
'Content must end with last line text (no trailing newline)'
303+
);
304+
console.log(' PASS');
305+
}
306+
307+
/**
308+
* Test 9: Partial read of file without trailing newline - mid-file read should still get newline
309+
*/
310+
async function testPartialReadNoTrailingNewlineFile() {
311+
console.log('\n Test 9: partial read of no-trailing-newline file still gets newline at mid-file');
312+
313+
// Read first 2 lines of the no-trailing-newline file
314+
const result = await handleReadFile({ path: CRLF_NO_TRAILING_FILE, offset: 0, length: 2 });
315+
const text = result.content[0].text;
316+
317+
const contentStart = text.indexOf('Line one');
318+
const fileContent = text.substring(contentStart);
319+
320+
// Even though the file doesn't end with a newline, our partial read
321+
// is in the middle of the file — the last line we read ("Line two")
322+
// definitely has a newline after it.
323+
assert.ok(
324+
fileContent.endsWith('\r\n'),
325+
'Partial read from middle of file must end with CRLF regardless of file trailing newline'
326+
);
327+
console.log(' PASS');
328+
}
329+
213330
async function runTests() {
214331
console.log('=== CRLF Preservation Tests (issue #97) ===');
215332
let originalConfig;
@@ -220,6 +337,10 @@ async function runTests() {
220337
await testReadFileLFUnchanged();
221338
await testEditBlockPreservesCRLF();
222339
await testReadWriteRoundtrip();
340+
await testPartialReadPreservesCRLF();
341+
await testPartialReadPreservesLF();
342+
await testNoTrailingNewlinePreserved();
343+
await testPartialReadNoTrailingNewlineFile();
223344
console.log('\n All CRLF preservation tests passed!\n');
224345
return true;
225346
} catch (error) {

0 commit comments

Comments
 (0)