fix: judge programmatic-I/O task outcomes by the output hook, not the exit code#2731
Merged
Conversation
… exit code (#2727) Exempting programmatic-I/O task types (layered-intelligence) from the [task-<id>] commit criterion in #2700 left them judged purely by exit code: an exit-0 run with a missing/malformed .agent-done sentinel, or whose output hook threw, was recorded as a SUCCESS in learning.json and inflated the type's success rate that readLiTaskMetrics/computeSelfEvalSummary report back. These tasks now declare their own success criterion — the sentinel parsed and the output hook accepted it. dispatchTaskOutputHook is hoisted above completeAgent so the verdict exists before the learning write, which is a no-op reorder for every other task shape (the hook only ever runs for a type that registers one). The completeAgent-before-updateTask ordering documented in finalizeAgent is preserved, the lane is already released by that point, and agent:completed still fires after any handoff the hook enqueues. The accept/reject classification delegates to resolveTypeFailureSignal, the same pure decision the #2616 type-level failure ledger uses, so the learning verdict and the ledger cannot drift on what counts as a bad run. A hook that never ran yields null (undeclared), not true — 'nothing evaluated the output' must not collapse into 'the output was accepted'. Pipeline/mediaJob are unchanged: they register no output hook, so there is no deliverable signal to judge them by and they stay exit-code-judged.
…t from an empty answer (#2727) Review findings on the hoist: - The hoist put the output hook ahead of completeAgent, which is where status flips off 'running' — the count the CoS concurrency gate reads. So an LI run held 1-of-3 slots for the hook's duration, and a HUNG hook (it shells out to gh/glab and can walk 50 sequential embeddings, none of it timeout-bounded) held one until restart, with the task stuck in_progress and the orphan reaper protecting the zombie because it filters on 'running' too. Dispatch is now hard-bounded; a timeout resolves to the no-verdict sentinel, not a rejection, so finalize proceeds and task-learning falls back to the exit code as it did pre-#2727. - A payload that parsed as JSON but isn't a reasoner envelope (bare string, number, array) reached reason='no-proposal' — the same reason a well-formed response that legitimately proposes nothing gets. Garbage was therefore indistinguishable from a correct empty answer and recorded as a successful run. Resolve the usable envelope once and key both the validation and the reason off it. - resolveTypeFailureSignal optional-chains past a missing/non-object outcome into its success default, so a hook returning undefined read as 'accepted'. Guard the outcome shape and the exit-code boolean: both now yield the undeclared sentinel rather than a fabricated verdict. Drops the unreachable 'skip' branch (it requires terminatedByUser, handled upstream). - Collapse the criterion gate and the hook-dispatch gate onto one resolver (resolveTaskHookType). They keyed on metadata.analysisType vs analysisType||taskType, so a task typed at the top level ran the hook but was still commit-checked — the #2700 bug one shape over. file-failed / tracker-read-failed stay successes, deliberately: the output was accepted and a forge outage is environmental. Blaming the run would tank the type's success rate and auto-park it whenever gh has a bad afternoon. Documented at the criterion and pinned by a test so it isn't re-litigated.
…ree success (#2727) Round-2 review findings: - The timeout callback logged BEFORE resolving. emitLog is a synchronous EventEmitter.emit with live listeners; a throw there would leave the race permanently unsettled — wedging the exact finalize the timer exists to rescue — and an uncaught throw in a setTimeout callback crashes the process. Resolve first, then log inside try/catch. - An object payload carrying none of the documented reasoner keys ({}, {"foo":1}) still reached reason='no-proposal' — the same reason a legitimate empty answer gets. Reachable: the sentinel envelope only requires payload to be an object, and salvageSentinelPayload's lenient extractor can surface a non-envelope object out of prose. Require a recognized key (Object.hasOwn, so an inherited key can't qualify). - The unparseable path still awaited readIssues — an unbounded forge call — before returning its already-known verdict. A hang there burned the finalize timeout and fell back to the exit code, recording the malformed sentinel as the success this whole issue is about. readIssues is now scoped to the has-a-proposal branch that actually consumes it, so the common no-op runs don't shell out to gh while holding a slot. - no-app / app-not-found return before the payload is validated and record nothing, yet read as success — banking a free win for the type whenever an app is deleted mid-run. They now yield the undeclared sentinel. - Order the thrown-hook guard ahead of the exit-code guard so a rejection can't be masked by a non-boolean success. Also: the two #2344 commit-criterion tests had been left inside the new resolveProgrammaticIoVerdict describe (which lacked clearAllMocks); moved them to their own block. Pinned withOutputHookTimeout directly — it mitigates a wedge-until-restart bug and was only covered indirectly. Documented why the #2616 ledger gate deliberately does NOT share resolveTaskHookType: it asks a different question, and the resolver's taskType fallback would ledger ad-hoc queue categories.
…t an empty answer (#2727) A reasoner that supplied a non-null proposal which then failed validation (missing/unknown scope, no title, unnormalizable slug) landed on reason='no-proposal' — the same reason an explicit proposal:null gets. So 'tried to propose and emitted the wrong shape' was recorded as 'looked and correctly found nothing', i.e. a successful run. Narrow by design: it reclassifies only the field that IS the deliverable, leaving validateReasonerResponse's documented lenient drop-invalid-pieces contract alone. Files the two remaining review findings as PLAN.md follow-ups rather than stretching this PR: a skip channel through recordTaskCompletion for hook aborts that never evaluated the output (today they fall back to the exit code and bank a success — but 'false' would blame the model for a deleted app), and whether a wrong-typed non-deliverable field should invalidate a whole envelope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2727
Summary
#2700 exempted programmatic-I/O task types (
layered-intelligence) from the[task-<id>]commit criterion — necessary, since those tasks are explicitly told not to commit. But with no criterion declared, the runs fell through to being judged purely by exit code: a run that exits 0 with a missing/malformed.agent-donesentinel, or whose output hook throws, was recorded as a success inlearning.json, inflating the success ratereadLiTaskMetrics/computeSelfEvalSummaryreport back.These tasks now declare their own criterion: the agent's structured output parsed and the output hook accepted it.
Design: option 1 (defer the telemetry write)
Chose option 1 over the two-phase "correct after the fact" record, because it turned out to carry a smaller blast radius than the issue anticipated:
dispatchTaskOutputHookis hoisted abovecompleteAgent, and its result feedsevaluateSuccessCriteria. That is a no-op reorder for every other task shape — the hook is already gated onisProgrammaticIoTaskType, so it only ever runs for a type that registers one; nothing else in the chain moves. That avoids inventing a two-phase record (a second write path intolearning.json, and a window where the aggregates are knowingly wrong).The ordering guarantees documented at
agentLifecycle.js:890-905are preserved:completeAgentstill precedesupdateTask, both still under thecosStatemutex; the lane is already released by this point (releaseAgentLanefires earlier, in the spawn paths); andagent:completed— which schedules the nextdequeueNextTask— still fires fromcompleteAgent, i.e. after any handoff task the hook enqueues.The hoist's one real cost, found in review:
status: 'running'is what the CoS concurrency gate counts (cos.js, default 3 slots), and that flips incompleteAgent. So awaiting the hook first means an LI run holds a slot for the hook's duration — and a hung hook (it shells out togh/glaband can walk 50 sequential embeddings, none of it timeout-bounded) would hold one until restart, with the task stuckin_progressand the orphan reaper protecting the zombie because it filters onrunningtoo. Dispatch is therefore hard-bounded bywithOutputHookTimeout; a timeout resolves to the no-verdict sentinel, not a rejection, so finalize proceeds and task-learning falls back to the exit code exactly as it did pre-#2727. The unbounded tracker read is also now scoped to the branch that consumes it, so the common no-op runs don't shell out toghwhile holding a slot at all.Reuse over reimplementation
The accept/reject classification delegates to
resolveTypeFailureSignal, the same pure decision the #2616 type-level failure ledger already uses — so the learning verdict and the ledger can't drift on what counts as a bad run.Per the sentinel-and-validate convention the criterion has three answers that are never collapsed: false (the hook ran and rejected the output), null (nothing evaluated it — no hook ran, it timed out, it returned no structured outcome, or it aborted before looking), true (the hook ran and accepted it). "Not evaluated" never becomes "accepted".
Review also surfaced that the same conflation this issue is about lived one level down in the LI hook: output that isn't a real answer — a bare scalar/array, an object with none of the documented keys (
{}, reachable because the sentinel envelope only requirespayloadto be an object, and the lenient salvage extractor can surface a non-envelope object from prose), or a supplied-but-invalidproposal— all landed onreason='no-proposal', the same reason a legitimate empty answer gets. Now told apart.metrics.jsneeded no change —outcomeSuccess = validationPassed ?? successwas already correct; it was being fed a null verdict. The added tests there pin that downstream contract.Deliberate calls
file-failed/tracker-read-failedremain successes. The agent's output was accepted; the forge was simply unreachable. That's environmental — blaming the run would tank the type's success rate and, through the shared classification, auto-park the whole task type every timeghhas a bad afternoon. Documented at the criterion and pinned by a test.pipeline/mediaJobunchanged. They register no output hook, so there is no deliverable signal to judge them by — bringing them under this model would mean fabricating one. They stay exit-code-judged, pinned by a test.resolveTaskHookType. That resolver falls back totask.taskType, which for an ad-hoc task is the CoS queue category — ledgering those would invent a failure ledger for a "task type" no schedule owns. Documented inline.metadata.analysisTypevsanalysisType || taskType, so a task typed at the top level ran the hook but was still commit-checked — the Add LI self-evaluation source for pre-filing proposal quality check #2700 bug one shape over. Collapsed onto one resolver.recordTaskCompletionfor hook aborts that never evaluated the output (todaynull→ falls back to the exit code → banks a success;falsewould blame the model for a deleted app), and whether a wrong-typed non-deliverable field should invalidate a whole envelope (would re-litigatevalidateReasonerResponse's deliberately lenient contract).Test plan
server/services/agentLifecycle.successCriteria.test.js— a#2727suite pinning each acceptance criterion (exit-0 + missing/malformed sentinel → failure; hook threw → failure; exit-0 + hook accepted → success,checkForTaskCommitnever called; benign reasons → success; non-zero exit → failure; no hook ran / user-terminated / hook aborted pre-evaluation →null), direct unit tests forresolveProgrammaticIoVerdict, and direct tests forwithOutputHookTimeout(fake timers: a hung dispatch resolves to the sentinel, an in-time hook passes through and clears its timer, a rejection propagates).server/services/autonomousJobs/layeredIntelligenceHooks.test.js— non-envelope payloads (scalar/array/{}/{foo:1}) and a supplied-but-invalid proposal →unparseable-response; a well-formed envelope proposing nothing (incl. explicitproposal: null) → stillno-proposal; tracker untouched on the no-proposal path, still read when there IS a proposal.server/services/taskLearning/metrics.test.js— pins the verdict landing in the learning outcome (outcomeSuccessfalse + failure signature harvested; true + none).agentLifecycle.jsand keeping the tests fails 6 of them.origin/mainbaseline (all Postgres-connectivity failures from no local test DB). This change introduces zero new failures. Affected suites: 220/220 pass.