fix: address remaining minor code review findings (FAR-15)
- #9: match Paperclip container by name in k8s-client instead of trusting spec.containers[0], which could be a service-mesh sidecar - #11: key assistant-text dedup by (message.id, index) so legitimate duplicate content across turns isn't collapsed in the summary - #16: trim trailing hyphens from sanitized K8s names so truncation doesn't produce names ending in "-" Findings #5 (keepalive re-verify) and #6 (one-shot log dedup) were already addressed in the current code — verified during this review. #8 (orphan reattach behavior) requires a product decision on whether "new session wins" is intentional, so deferring. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
@@ -203,7 +203,9 @@ export interface JobBuildResult {
|
|||||||
}
|
}
|
||||||
|
|
||||||
function sanitizeForK8sName(value: string, maxLen = 16): string {
|
function sanitizeForK8sName(value: string, maxLen = 16): string {
|
||||||
return value.toLowerCase().replace(/[^a-z0-9-]/g, "").slice(0, maxLen);
|
// Trim trailing hyphens after slicing so names don't end with `-` when
|
||||||
|
// truncation lands on a hyphen boundary (finding #16, FAR-15).
|
||||||
|
return value.toLowerCase().replace(/[^a-z0-9-]/g, "").slice(0, maxLen).replace(/-+$/, "");
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -106,7 +106,12 @@ export async function getSelfPodInfo(kubeconfigPath?: string): Promise<SelfPodIn
|
|||||||
throw new Error(`claude_k8s: pod ${hostname} has no spec`);
|
throw new Error(`claude_k8s: pod ${hostname} has no spec`);
|
||||||
}
|
}
|
||||||
|
|
||||||
const mainContainer = spec.containers[0];
|
// Match the Paperclip container by name ("paperclip") to avoid service-mesh
|
||||||
|
// sidecars or other injected containers being picked up as the source of
|
||||||
|
// truth for the Job spec (finding #9, FAR-15). Fall back to the first
|
||||||
|
// container if no name match is found (matches prior behavior).
|
||||||
|
const mainContainer =
|
||||||
|
spec.containers.find((c) => c.name === "paperclip") ?? spec.containers[0];
|
||||||
if (!mainContainer?.image) {
|
if (!mainContainer?.image) {
|
||||||
throw new Error(`claude_k8s: pod ${hostname} has no container image`);
|
throw new Error(`claude_k8s: pod ${hostname} has no container image`);
|
||||||
}
|
}
|
||||||
|
|||||||
+15
-6
@@ -9,9 +9,12 @@ export function parseClaudeStreamJson(stdout: string) {
|
|||||||
let model = "";
|
let model = "";
|
||||||
let finalResult: Record<string, unknown> | null = null;
|
let finalResult: Record<string, unknown> | null = null;
|
||||||
const assistantTexts: string[] = [];
|
const assistantTexts: string[] = [];
|
||||||
// Belt-and-braces dedup: track seen text blocks to filter duplicates
|
// Belt-and-braces dedup: key by (message.id, textIndex) so a session that
|
||||||
// caused by log stream reconnects replaying overlapping windows.
|
// legitimately emits the same text twice in different turns isn't collapsed
|
||||||
const seenTexts = new Set<string>();
|
// (finding #11, FAR-15). The log-dedup filter handles reconnect overlaps
|
||||||
|
// at the line level; this guard only needs to protect against the same
|
||||||
|
// message block being parsed twice.
|
||||||
|
const seenBlocks = new Set<string>();
|
||||||
|
|
||||||
for (const rawLine of stdout.split(/\r?\n/)) {
|
for (const rawLine of stdout.split(/\r?\n/)) {
|
||||||
const line = rawLine.trim();
|
const line = rawLine.trim();
|
||||||
@@ -29,14 +32,20 @@ export function parseClaudeStreamJson(stdout: string) {
|
|||||||
if (type === "assistant") {
|
if (type === "assistant") {
|
||||||
sessionId = asString(event.session_id, sessionId ?? "") || sessionId;
|
sessionId = asString(event.session_id, sessionId ?? "") || sessionId;
|
||||||
const message = parseObject(event.message);
|
const message = parseObject(event.message);
|
||||||
|
const messageId = asString(message.id, "");
|
||||||
const content = Array.isArray(message.content) ? message.content : [];
|
const content = Array.isArray(message.content) ? message.content : [];
|
||||||
for (const entry of content) {
|
for (let i = 0; i < content.length; i++) {
|
||||||
|
const entry = content[i];
|
||||||
if (typeof entry !== "object" || entry === null || Array.isArray(entry)) continue;
|
if (typeof entry !== "object" || entry === null || Array.isArray(entry)) continue;
|
||||||
const block = entry as Record<string, unknown>;
|
const block = entry as Record<string, unknown>;
|
||||||
if (asString(block.type, "") === "text") {
|
if (asString(block.type, "") === "text") {
|
||||||
const text = asString(block.text, "");
|
const text = asString(block.text, "");
|
||||||
if (text && !seenTexts.has(text)) {
|
if (!text) continue;
|
||||||
seenTexts.add(text);
|
// Prefer (messageId, index) when the message has an id; fall back
|
||||||
|
// to text content when it doesn't (legacy/partial events).
|
||||||
|
const key = messageId ? `${messageId}:${i}` : `text:${text}`;
|
||||||
|
if (!seenBlocks.has(key)) {
|
||||||
|
seenBlocks.add(key);
|
||||||
assistantTexts.push(text);
|
assistantTexts.push(text);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user