Skip to content

Commit 2881cca

Browse files
committed
Fix dead UTF-8 validation check and PR failure losing commit info
Bug 1: Replace no-op Buffer.from try/catch with real unpaired surrogate detection. Buffer.from(string, utf-8) never throws for JS strings, making the previous check dead code. The new regex detects lone surrogates which are valid in JS strings but not valid UTF-8. Bug 2: Wrap PR creation in its own try/catch so a PR failure (network, permissions, rate limit) does not lose the successful commit metadata. Previously commitResult was scoped inside try and inaccessible from catch, causing the error response to report Write failed with no commit info.
1 parent e324ff0 commit 2881cca

2 files changed

Lines changed: 23 additions & 19 deletions

File tree

src/core/actions.js

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -623,18 +623,24 @@ export async function handleAction(params) {
623623

624624
// --- Handle PR if requested (Tier 3) ---
625625
let prResult = null;
626+
let prError = null;
626627
// TODO: Orphan prevention (Layer 4) — before creating a new PR, check for
627628
// existing open PRs from oddkit on the same branch or targeting the same files.
628629
// If found, push to the existing branch instead (the PR updates automatically).
629630
// The output interface supports this via pr_updated. Deferred until Layer 4.
630631
if (pr && branch) {
631-
const prOpts = typeof pr === "object" ? pr : {};
632-
const prTitle = prOpts.title || message;
633-
const prBody = prOpts.body || `Files:\n${files.map((f) => `- ${f.path}`).join("\n")}\n\n---\nWritten via oddkit_write`;
634-
const prDraft = prOpts.draft || false;
635-
const baseBranch = defaultBranch || await getDefaultBranch(owner, repoName);
636-
prResult = await createPR(owner, repoName, prTitle, prBody, branch, baseBranch, prDraft);
637-
status = "pr_opened";
632+
try {
633+
const prOpts = typeof pr === "object" ? pr : {};
634+
const prTitle = prOpts.title || message;
635+
const prBody = prOpts.body || `Files:\n${files.map((f) => `- ${f.path}`).join("\n")}\n\n---\nWritten via oddkit_write`;
636+
const prDraft = prOpts.draft || false;
637+
const baseBranch = defaultBranch || await getDefaultBranch(owner, repoName);
638+
prResult = await createPR(owner, repoName, prTitle, prBody, branch, baseBranch, prDraft);
639+
status = "pr_opened";
640+
} catch (prErr) {
641+
prError = prErr.message;
642+
status = "committed_pr_failed";
643+
}
638644
}
639645

640646
const filesWritten = files.map((f) => f.path);
@@ -652,10 +658,11 @@ export async function handleAction(params) {
652658
files_written: filesWritten,
653659
pr_url: prResult?.pr_url || undefined,
654660
pr_number: prResult?.pr_number || undefined,
661+
pr_error: prError || undefined,
655662
pr_updated: false, // TODO: set to true when orphan prevention detects existing PR
656663
validation,
657664
},
658-
assistant_text: `Successfully wrote ${filesWritten.length} file(s) to ${owner}/${repoName} on branch ${targetBranch}. Commit: ${commitResult.commit_url}${prResult ? `\nPR: ${prResult.pr_url}` : ""}${validationWarnings ? `\n\nValidation warnings: ${validationWarnings}` : ""}`,
665+
assistant_text: `Successfully wrote ${filesWritten.length} file(s) to ${owner}/${repoName} on branch ${targetBranch}. Commit: ${commitResult.commit_url}${prResult ? `\nPR: ${prResult.pr_url}` : ""}${prError ? `\nPR creation failed: ${prError}` : ""}${validationWarnings ? `\n\nValidation warnings: ${validationWarnings}` : ""}`,
659666
debug: makeDebug({ files_count: files.length, tier: files.length === 1 ? 1 : 2, validation_passed: validation.passed }),
660667
};
661668

src/utils/writeValidation.js

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,17 +51,14 @@ export function validateFile(path, content) {
5151
: null,
5252
});
5353

54-
// UTF-8 validity
55-
try {
56-
Buffer.from(content, "utf-8");
57-
checks.push({ name: "utf8_valid", passed: true });
58-
} catch {
59-
checks.push({
60-
name: "utf8_valid",
61-
passed: false,
62-
message: "Content is not valid UTF-8",
63-
});
64-
}
54+
// UTF-8 validity — detect unpaired surrogates (valid in JS strings but not in UTF-8)
55+
const hasLoneSurrogates =
56+
/[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]/.test(content);
57+
checks.push({
58+
name: "utf8_valid",
59+
passed: !hasLoneSurrogates,
60+
message: hasLoneSurrogates ? "Content contains unpaired surrogate characters (not valid UTF-8)" : null,
61+
});
6562

6663
// Governed file checks (canon/, odd/, docs/)
6764
const isGoverned = path.startsWith("canon/") || path.startsWith("odd/") || path.startsWith("docs/");

0 commit comments

Comments
 (0)