mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 13:26:04 -06:00
fix(acp): classify complete task output before summary truncation (#129255)
* fix(tasks): detect sentence boundaries without space after terminator hasNonProgressFollowupSentence only recognized a sentence boundary when the terminator was followed by whitespace. Glued ACP outputs (e.g. "...behavior.There's no unit-test target yet...") never matched, so a complete final report following progress narration was classified as progress-only and the required completion was reported as blocked. Insert the missing space after [.!?:] when followed directly by an uppercase letter before running boundary detection, and add regression tests. Closes #129222 * fix(tasks): exclude structured colon tokens from sentence-boundary normalization * fix(tasks): keep dotted references glued in sentence-boundary normalization The uppercase lookahead normalization inserted a space after any dot followed by an uppercase letter, which fabricated sentence boundaries inside dotted structured references. "I'll inspect API.Client" became "I'll inspect API. Client": the first fragment matched the progress prefix and "Client" was treated as a final deliverable, turning a blocked progress-only result into a false completed verdict. Skip the space insertion when the dot sits inside a dotted identifier/path token (identifier chars around the dot, no whitespace in the token) or is followed by a known file extension (Kt, py, js, ts, json, yaml, toml, sh, mjs, cjs, go, rs, java, cs, h, cpp). Real glued sentence boundaries ("repo.There's...") still get the space and are still split. Add regressions for API.Client, foo.Bar and src/main.Kt / config.py, which are red on the pre-fix head and green now. * fix(tasks): recognize glued prose before dotted-reference exemptions The whole-token dotted-reference exemption also preserved glued prose boundaries such as hooks.Targets and suite.PinPoints from the reported #129222 transcript, so their missing spaces were never inserted and the contract only reached a completed result incidentally via a later spaced boundary. insertMissingSentenceSpaces now splits a dot glued to a Capitalized word after a lowercase word when the continuation starts a sentence, and keeps glued only structured dotted references: known extensions, paths, acronyms, multi-dot tokens, and camelCase identifiers followed by planning narration or punctuation. The normalizer is exported and asserted directly in tests, and punctuated dotted references (API.Client,) stay progress-only. * fix(tasks): preserve ordinary dotted progress references; privatize normalizer * fix(tasks): recognize glued prose boundaries before ordinary-continuation exemption The lowercase-continuation exemption from the previous round treated every plain post-dot word followed by lowercase text as a structured dotted token, which also swallowed the reported glued prose boundaries (issue #129222): 'hooks.Targets are wired' and 'suite.PinPoints now has' were left glued, so a report whose only boundary is one of these stayed progress-only and blocked. Only clause-opening continuations now split: copulas/auxiliaries, or the sentence adverb 'now' before a finite verb. Ordinary noun-phrase continuations ('foo.Bar results') still keep the dotted reference glued, so progress narration is never split into a fake deliverable. * fix(tasks): recognize general now+finite-verb glued prose boundaries (#129222) Generalize the now-branch of the sentence-opening continuation pattern from a closed auxiliary/copula whitelist to any finite verb that is not a closed-class function word, so reports like 'suite.PinPoints now works.' split at the glued boundary instead of being misread as a final deliverable. Regression tests for general and inflected finite verbs (red -> green). * fix(acp): classify complete task output before truncation Co-authored-by: Finn763 <165816600+Finn763@users.noreply.github.com> * fix(acp): count joined completion evidence bytes Co-authored-by: Finn763 <165816600+Finn763@users.noreply.github.com> --------- Co-authored-by: Finn763 <Finn763@users.noreply.github.com> Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
This commit is contained in:
Finn763
@@ -72,16 +72,16 @@ export function resolveBackgroundTaskFailureStatus(error: AcpRuntimeError): "fai
|
||||
return error.detailCode === ACP_TURN_TIMEOUT_DETAIL_CODE ? "timed_out" : "failed";
|
||||
}
|
||||
|
||||
/** Infers blocked terminal outcomes from final progress text when the child turn reports one. */
|
||||
export function resolveBackgroundTaskTerminalResult(progressSummary: string): {
|
||||
/** Infers blocked terminal outcomes from final completion text when the child turn reports one. */
|
||||
export function resolveBackgroundTaskTerminalResult(completionText: string): {
|
||||
terminalOutcome?: "blocked";
|
||||
terminalSummary?: string;
|
||||
} {
|
||||
const requiredCompletionResult = resolveRequiredCompletionTerminalResult(progressSummary);
|
||||
const requiredCompletionResult = resolveRequiredCompletionTerminalResult(completionText);
|
||||
if (requiredCompletionResult.terminalOutcome) {
|
||||
return requiredCompletionResult;
|
||||
}
|
||||
const normalized = normalizeText(progressSummary)?.replace(/\s+/g, " ").trim();
|
||||
const normalized = normalizeText(completionText)?.replace(/\s+/g, " ").trim();
|
||||
if (!normalized) {
|
||||
return {};
|
||||
}
|
||||
|
||||
@@ -539,22 +539,24 @@ describe("AcpSessionManager turn results", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps parented ACP turns successful when final output follows progress text", async () => {
|
||||
it("classifies complete parented ACP output before truncating its progress summary", async () => {
|
||||
await withAcpManagerTaskStateDir(async () => {
|
||||
const runtimeState = createRuntime();
|
||||
const completionChunks = [
|
||||
"I'll run the unit tests against existing models and launch-argument hooks.",
|
||||
"Targets are wired in. Next I'll run the unit tests on a booted simulator, then the UI smoke suite.",
|
||||
"PinPoints now has a real XCTest unit-test target and a deterministic UI smoke suite. Product code was not changed.",
|
||||
];
|
||||
runtimeState.runtime.startTurn = vi.fn((input) => ({
|
||||
requestId: input.requestId,
|
||||
events: (async function* () {
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text: "I'll inspect the repo now. ",
|
||||
};
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text: "The crash is a missing null check in src/foo.ts.",
|
||||
};
|
||||
for (const text of completionChunks) {
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text,
|
||||
};
|
||||
}
|
||||
})(),
|
||||
result: Promise.resolve({
|
||||
status: "completed" as const,
|
||||
@@ -577,7 +579,7 @@ describe("AcpSessionManager turn results", () => {
|
||||
sessionId: "child-1",
|
||||
updatedAt: Date.now(),
|
||||
spawnedBy: "agent:quant:telegram:quant:direct:822430204",
|
||||
label: "Progress then final",
|
||||
label: "Complete output",
|
||||
},
|
||||
acp: readySessionMeta(),
|
||||
};
|
||||
@@ -602,19 +604,19 @@ describe("AcpSessionManager turn results", () => {
|
||||
sessionKey: "agent:codex:acp:child-1",
|
||||
text: "Inspect and report back",
|
||||
mode: "prompt",
|
||||
requestId: "direct-parented-progress-then-final-run",
|
||||
requestId: "direct-parented-complete-output-run",
|
||||
});
|
||||
|
||||
const record = requireTaskByRunId("direct-parented-progress-then-final-run");
|
||||
const record = requireTaskByRunId("direct-parented-complete-output-run");
|
||||
expectRecordFields(record, {
|
||||
runtime: "acp",
|
||||
ownerKey: "agent:quant:telegram:quant:direct:822430204",
|
||||
scopeKind: "session",
|
||||
childSessionKey: "agent:codex:acp:child-1",
|
||||
status: "succeeded",
|
||||
progressSummary:
|
||||
"I'll inspect the repo now. The crash is a missing null check in src/foo.ts.",
|
||||
});
|
||||
expect(record.progressSummary).toHaveLength(240);
|
||||
expect(record.progressSummary).toMatch(/…$/);
|
||||
expect(record.terminalOutcome).toBeUndefined();
|
||||
expect(record.terminalSummary).toBeUndefined();
|
||||
});
|
||||
@@ -774,13 +776,16 @@ describe("AcpSessionManager turn results", () => {
|
||||
it("marks completed parented ACP turns blocked when they only contain progress text", async () => {
|
||||
await withAcpManagerTaskStateDir(async () => {
|
||||
const runtimeState = createRuntime();
|
||||
const progressOnly = "I'll inspect the repo, run the tests, and report back. "
|
||||
.repeat(8)
|
||||
.trim();
|
||||
runtimeState.runtime.startTurn = vi.fn((input) => ({
|
||||
requestId: input.requestId,
|
||||
events: (async function* () {
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text: "I'll inspect the repo now.",
|
||||
text: progressOnly,
|
||||
};
|
||||
})(),
|
||||
result: Promise.resolve({
|
||||
@@ -837,17 +842,94 @@ describe("AcpSessionManager turn results", () => {
|
||||
});
|
||||
|
||||
expect(events).toEqual(["text_delta", "done"]);
|
||||
expectRecordFields(requireTaskByRunId("direct-parented-progress-completed-run"), {
|
||||
const record = requireTaskByRunId("direct-parented-progress-completed-run");
|
||||
expectRecordFields(record, {
|
||||
runtime: "acp",
|
||||
ownerKey: "agent:quant:telegram:quant:direct:822430204",
|
||||
scopeKind: "session",
|
||||
childSessionKey: "agent:codex:acp:child-1",
|
||||
status: "succeeded",
|
||||
progressSummary: "I'll inspect the repo now.",
|
||||
terminalOutcome: "blocked",
|
||||
terminalSummary:
|
||||
"Required completion ended with progress-only text, not a final deliverable.",
|
||||
});
|
||||
expect(record.progressSummary).toHaveLength(240);
|
||||
expect(record.progressSummary).toMatch(/…$/);
|
||||
});
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
label: "accepts exact-limit evidence when a surrogate pair spans deltas",
|
||||
requestId: "direct-parented-completion-exact-limit-run",
|
||||
suffix: "",
|
||||
overflowed: false,
|
||||
},
|
||||
{
|
||||
label: "blocks parented ACP completion evidence one byte over the verification limit",
|
||||
requestId: "direct-parented-completion-overflow-run",
|
||||
suffix: "x",
|
||||
overflowed: true,
|
||||
},
|
||||
])("$label", async ({ requestId, suffix, overflowed }) => {
|
||||
await withAcpManagerTaskStateDir(async () => {
|
||||
const runtimeState = createRuntime();
|
||||
const prefix = "Final report: ";
|
||||
const filler = "x".repeat(100 * 1024 - Buffer.byteLength(prefix, "utf8") - 4);
|
||||
runtimeState.runtime.startTurn = vi.fn((input) => ({
|
||||
requestId: input.requestId,
|
||||
events: (async function* () {
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text: `${prefix}${filler}\ud83d`,
|
||||
};
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text: `\ude00${suffix}`,
|
||||
};
|
||||
})(),
|
||||
result: Promise.resolve({
|
||||
status: "completed" as const,
|
||||
stopReason: "end_turn",
|
||||
}),
|
||||
cancel: vi.fn(async () => {}),
|
||||
closeStream: vi.fn(async () => {}),
|
||||
}));
|
||||
hoisted.requireAcpRuntimeBackendMock.mockReturnValue({
|
||||
id: "acpx",
|
||||
runtime: runtimeState.runtime,
|
||||
});
|
||||
mockParentedAcpSessionEntries({
|
||||
childSessionKey: "agent:codex:acp:child-1",
|
||||
parentSessionKey: "agent:main:main",
|
||||
});
|
||||
|
||||
const manager = new AcpSessionManager();
|
||||
await manager.runTurn({
|
||||
provenance: "system",
|
||||
cfg: baseCfg,
|
||||
sessionKey: "agent:codex:acp:child-1",
|
||||
text: "Produce a final report",
|
||||
mode: "prompt",
|
||||
requestId,
|
||||
});
|
||||
|
||||
const record = requireTaskByRunId(requestId);
|
||||
expectRecordFields(record, {
|
||||
ownerKey: "agent:main:main",
|
||||
childSessionKey: "agent:codex:acp:child-1",
|
||||
status: "succeeded",
|
||||
});
|
||||
expect(record.terminalOutcome).toBe(overflowed ? "blocked" : undefined);
|
||||
expect(record.terminalSummary).toBe(
|
||||
overflowed
|
||||
? "Required completion output exceeded the 100 KB verification limit; inspect the child session for the final deliverable."
|
||||
: undefined,
|
||||
);
|
||||
expect(record.progressSummary).toHaveLength(240);
|
||||
expect(record.progressSummary).toMatch(/…$/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1020,7 +1102,11 @@ describe("AcpSessionManager turn results", () => {
|
||||
runtimeState.runtime.startTurn = vi.fn((input) => ({
|
||||
requestId: input.requestId,
|
||||
events: (async function* () {
|
||||
yield { type: "text_delta" as const, stream: "output" as const, text: "stopping" };
|
||||
yield {
|
||||
type: "text_delta" as const,
|
||||
stream: "output" as const,
|
||||
text: "I'll inspect the repo, run the tests, and report back.",
|
||||
};
|
||||
})(),
|
||||
result: Promise.resolve({
|
||||
status: "cancelled" as const,
|
||||
@@ -1055,11 +1141,15 @@ describe("AcpSessionManager turn results", () => {
|
||||
expect(closeStream).toHaveBeenCalledWith({ reason: "turn-result-cancelled" });
|
||||
expect(events.map((event) => event.type)).toEqual(["text_delta", "done"]);
|
||||
expect(events.at(-1)).toEqual({ type: "done", status: "cancelled" });
|
||||
expectRecordFields(requireTaskByRunId("run-1"), {
|
||||
const record = requireTaskByRunId("run-1");
|
||||
expectRecordFields(record, {
|
||||
ownerKey: "agent:main:main",
|
||||
childSessionKey: "agent:codex:acp:child-1",
|
||||
status: "cancelled",
|
||||
progressSummary: "I'll inspect the repo, run the tests, and report back.",
|
||||
});
|
||||
expect(record.terminalOutcome).toBeUndefined();
|
||||
expect(record.terminalSummary).toBeUndefined();
|
||||
const states = extractStatesFromUpserts();
|
||||
expect(states).toContain("running");
|
||||
expect(states).toContain("idle");
|
||||
|
||||
@@ -46,7 +46,7 @@ import type {
|
||||
import { normalizeActorKey, requireReadySessionMeta } from "./manager.utils.js";
|
||||
|
||||
const ACP_TURN_TIMEOUT_GRACE_MS = 1_000;
|
||||
|
||||
const ACP_COMPLETION_EVIDENCE_MAX_BYTES = 100 * 1024;
|
||||
type ApplyRuntimeControls = (params: {
|
||||
sessionKey: string;
|
||||
runtime: AcpRuntime;
|
||||
@@ -212,6 +212,9 @@ export async function runManagerTurn(params: {
|
||||
let sawTurnOutput = false;
|
||||
let retryFreshHandle = false;
|
||||
let skipPostTurnCleanup = false;
|
||||
let completionEvidenceText = "";
|
||||
let completionEvidenceBytes = 0;
|
||||
let completionEvidenceOverflowed = false;
|
||||
try {
|
||||
const ensured = await params.ensureRuntimeHandle({
|
||||
cfg: input.cfg,
|
||||
@@ -255,7 +258,6 @@ export async function runManagerTurn(params: {
|
||||
};
|
||||
params.activeTurnBySession.set(actorKey, activeTurn);
|
||||
activeTurnStarted = true;
|
||||
|
||||
const combinedSignal = input.signal
|
||||
? AbortSignal.any([input.signal, internalAbortController.signal])
|
||||
: internalAbortController.signal;
|
||||
@@ -297,6 +299,15 @@ export async function runManagerTurn(params: {
|
||||
taskProgressSummary,
|
||||
event.text,
|
||||
);
|
||||
// Keep semantic evidence attempt-local; only the bounded display summary is persisted.
|
||||
if (taskContext && !completionEvidenceOverflowed) {
|
||||
completionEvidenceText += event.text;
|
||||
completionEvidenceBytes = Buffer.byteLength(completionEvidenceText, "utf8");
|
||||
if (completionEvidenceBytes > ACP_COMPLETION_EVIDENCE_MAX_BYTES) {
|
||||
completionEvidenceOverflowed = true;
|
||||
completionEvidenceText = "";
|
||||
}
|
||||
}
|
||||
}
|
||||
if (taskContext) {
|
||||
markBackgroundTaskRunning(taskContext.runId, {
|
||||
@@ -347,7 +358,16 @@ export async function runManagerTurn(params: {
|
||||
startedAt: turnStartedAt,
|
||||
});
|
||||
if (taskContext) {
|
||||
const terminalResult = resolveBackgroundTaskTerminalResult(taskProgressSummary);
|
||||
const terminalResult =
|
||||
turnOutcome.terminalStatus === "cancelled"
|
||||
? {}
|
||||
: completionEvidenceOverflowed
|
||||
? {
|
||||
terminalOutcome: "blocked" as const,
|
||||
terminalSummary:
|
||||
"Required completion output exceeded the 100 KB verification limit; inspect the child session for the final deliverable.",
|
||||
}
|
||||
: resolveBackgroundTaskTerminalResult(completionEvidenceText);
|
||||
markBackgroundTaskTerminal(taskContext.runId, {
|
||||
sessionKey,
|
||||
status: turnOutcome.terminalStatus === "cancelled" ? "cancelled" : "succeeded",
|
||||
|
||||
Reference in New Issue
Block a user