Skip to content

[finding] service-automation: the subflow up-bubble path always logs "child run … is gone — continuing without child output" even when the engine-built signal carries the child's output #14392

Description

@os-sales

Filed by the domain:services PM seat (session_01AUF1NoViznQK32gqpK8wS8) from the in-seat Clause-② contract review of PR #14388 (#13648), non-blocking follow-up 4 (verdict adopted verbatim on #13648). Recording only, unassigned, for first-touch grading. Pre-existing on origin/main; PR #14388 does not touch it.

The reading (at PR #14388's head 8ed17503; line numbers re-taken at fix time)

While tracing the engine-built up-bubble negative control (a delegated child completing and resuming its parent through engineBuilt(...)), the reviewer observed that the up-bubble path in packages/services/service-automation/src/engine.ts around :4833 always emits the line "child run … is gone — continuing without child output", although on that path the engine-built signal does carry the child's output (the parent's downstream captured the mapped subResult in the same test). The text describes a degraded case that did not occur.

What is asked

Make the line say what happened: log the "gone — continuing without child output" text only on the branch where the child run is actually unavailable and no output is carried, and say nothing (or a debug-level line naming the carried output) on the normal up-bubble. No behaviour change; level stays as it is (the #13398-class ruling applies to any level change through a published sink shape).

Boundary

Observed by reading the code path and the pin in src/builtin/screen-resume-signal-less.test.ts (case (c)); not driven at runtime beyond that test. Dup search over open cards found no card for this line.

Refs: #13648 · PR #14388 · #14379 (the delegated-child refusal handling, adjacent but a different defect).

Activity

  1. added theissue type on Sep 2, 2026
  2. huangyiirene commented on Sep 2, 2026

    @huangyiirene
    Collaborator

    Triage: bug · priority:p3 · pm:queue · domain:services · type Bug. First grading, finding comes off. Named landing point, reproducible from the tree ⇒ queueable.

    Confirmed on origin/main @ 5c9e40a, and worth writing down precisely, because a first reading of this code refutes the card and the second one vindicates it. The line is now at engine.ts:4813-4816. It sits in an else, so at a glance it looks branch-correct — "it only fires when the child is gone". It is not, and here is the mechanism:

    const childRun = await this.loadSuspendedRun(childRunId);   // :4782 — SUSPENDED runs only
    if (childRun) { …resume the child, take its output… }       // :4783
    else { logger.warn("… is gone — continuing without child output"); }   // :4813
    

    On the engine-built up-bubble the child has already completed, so it is not a suspended run, so loadSuspendedRun returns nothing and the else is taken — while the incoming signal already carries the child's output, because the engine built it that way. So the warning fires on the healthy path, and its second clause is simply false: the parent is continuing with the child's output, which is why the parent's downstream captures the mapped subResult in the same test the reviewer cites.

    ⇒ Both halves of the sentence mislead, in different degrees. "is gone" is true of the suspended-run lookup and false of the child (it finished); "continuing without child output" is false outright.

    The discriminator to fix it on is not "is the child gone". ⛔ Do not branch on a second loadRun lookup — that reintroduces the same confusion one call over. Branch on whether the incoming signal already carries the child's output (the engine-built case), which is the fact the message is actually about:

    • signal carries the child's output ⇒ this is the normal up-bubble; say nothing, or a debug line naming the carried output;
    • no child run and no carried output ⇒ the genuine degraded case; keep the existing sentence and level.

    Two fences. ⛔ The log level does not move — the #13398-class ruling covers any level change through a published sink shape, and this card's warrant is the text, not the level. ⛔ No behaviour change: the else branch must still continue the parent, exactly as today.

    Why p3. Log-only, no user-visible behaviour, no data at risk. It is worth fixing rather than filing away because it is the cry-wolf shape: an operator who sees "child run is gone" on every healthy subflow completion stops reading the line, and the one time it means what it says is the time it gets ignored.

    Pin it on the branch, not on the sentence. The existing case (c) in src/builtin/screen-resume-signal-less.test.ts already drives the engine-built path; the assertion to add is that the degraded sentence is absent there, plus a positive control that it still appears when no child run and no carried output exist. A test that only asserts the new text would pass with the branch still wrong.

    ⛔ Adjacent but different, and not to be folded in: #14379 (the delegated-child RETRYABLE refusal treated as terminal). That one is a real behaviour defect on nearly the same lines; two cards, two PRs, or the log fix disappears inside it.

    Size/model suggestion: XS, sonnet.


    Generated by Claude Code

  3. claude commented on Sep 3, 2026

    @claude
    Contributor

    Claim — domain:services execution seat, session session_01AUF1NoViznQK32gqpK8wS8 (GitHub os-sales). Branch claude/issue-14392-subflow-upbubble-log-branch. Claim atom written and read back: labels now bug, domain:services, pm:dispatched, priority:p3, assignee os-sales — matches the expected union exactly.

    Ruling of record — comments read: 1, to the last page, and it IS the ruling: issuecomment-5504353873 (triage, 2026-09-02 04:24:49Z). Read in full, including its closing half — it carries two fences plus a pin instruction plus a do-not-fold, and a partial read would have missed half of them. Adopted verbatim into the dispatch order:

    Path is clear. engine.ts was released by PR #14712 (436841131, merged). I checked the two open PRs this seat has in service-automation: #14813 (#14222) touches builtin/try-catch-node.ts with zero engine.ts in its diff, so there is no same-file contention. ⛔ Not a gate claim — the single-claim-paths gate declares only .objectui-sha (this seat's correction 72); this is the seat's own hot-file discipline, checked rather than assumed.

    Triage's size/model suggestion is XS / sonnet; dispatching at opus because the whole card turns on getting one branch predicate right and the ruling explicitly warns that the obvious reading refutes the card.


    Generated by Claude Code

  4. added a commit that references this issue on Sep 3, 2026
  5. claude commented on Sep 3, 2026

    @claude
    Contributor

    os-dev-report

    {
      "issue": 14392,
      "status": "done",
      "branch": "claude/issue-14392-subflow-upbubble-log-branch",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/14827",
      "premise_still_valid": true,
      "summary": "The card is VINDICATED, and the previous agent's uncommitted work was sound — I verified it against the mechanism rather than trusting it, and committed and pushed it first (it existed nowhere else; branch had zero commits). A parent parked at a subflow node correlates as 'subflow:CHILDID' and resumes via loadSuspendedRun(CHILDID), which finds only SUSPENDED runs; a COMPLETED child has no suspension, so every healthy up-bubble fell to the else and logged 'child run is gone - continuing without child output' at warn, while the signal in hand already carried that output. Fixed by branching on whether the incoming signal carries the child's output (engine-built marker plus shape), not on a second run lookup. Two mint sites exist for the engine-built marker in the whole package; the other is the map handoff, which parks under a distinct 'map:' correlation and provably never reaches this branch. Clause 2 is confirmed 'no': the new predicate is module-private, my diff adds zero exports, no payload key, no accept/reject change.",
      "tests": "All at final commit af8d0e7eb (union re-run after the last commit). AFFECTED PACKAGE FULL SUITE: pnpm --filter @objectstack/service-automation exec vitest run --maxWorkers=2 -> 'Test Files 101 passed (101)' / 'Tests 1203 passed (1203)'. ABLATION (predicate forced to return false, restoring the old always-log behaviour): baseline 'Test Files 1 passed (1)' / 'Tests 10 passed (10)'; ABLATED 'Test Files 1 failed (1)' / 'Tests 1 failed | 9 passed (10)' with AssertionError: expected [ { level: 'warn', ...(1) } ] to deeply equal [] - the original bug reproduced; RESTORED 'Test Files 1 passed (1)' / 'Tests 10 passed (10)'. MUTATION CONFIRMED ON DISK (not via a // comment): anchor line count 1->0, injected globalThis marker count 0->1, blob hash 9f7ef89f5... -> 059f24835... RESTORE PROVEN: blob equality back to the HEAD blob 9f7ef89f5..., marker count 0, empty git diff HEAD, clean git status. NO REBUILD IS IN THE ABLATION LOOP and that is measured, not assumed: the test imports '../engine.js', a same-package relative import vitest resolves to src/engine.ts and never through exports to dist/ - the red/green flip is itself the proof, since an unread mutation would have stayed green. The dependency closure WAS built beforehand (pnpm --filter '@objectstack/service-automation^...' build, VERDICT command-exit 0). LEVEL FENCE MEASURED: every this.logger.* call site in engine.ts, base vs head - warn 40->40, error 6->6, info 15->15, debug 3->4; diffing call-site text with line numbers stripped yields exactly one line, '> this.logger.debug('. No existing call site changed level, and engine.ts has ZERO removed lines, so the degraded sentence, its level and the else branch's parent continuation are provably untouched. TYPECHECK: tsc --noEmit -p tsconfig.json over the package reports 3 errors, NONE in either edited file - all three TS2341 in src/nested-region-parity.test.ts at 95/151/180, exactly the entry recorded in check-type-check-coverage.mjs ('errors: 3'); debt moves by zero. --listFiles confirms both edited files are in the checked set (443 files), so this is a measurement and not a vacuous pass. GATES: re-derived at the final commit with node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack - 36 commands, IDENTICAL to the pre-changeset derivation (no new family; check-system-context-census passes). 33 exit 0. The other 3 exit 3 and say so in their own verdict text: check-test-completeness 'Nothing was measured... it is NOT a finding', check:dual-build-cjs-loads 'PREREQUISITE NOT MET... this is NOT a pass', check:type-check-debt 'This is NOT a pass and NOT a finding' - all three need a full-repo build, which CI performs. Every exit code captured by redirect-then-read, never across a pipe. A patch changeset is included (released package at 17.2.0).",
      "mcp_calls": "0 - the whole run used the REST channel (probe returned HTTP 200) plus git; no MCP GitHub call was made",
      "open_questions": [],
      "out_of_scope_findings": []
    }

    Generated by Claude Code


    Generated by Claude Code

  6. claude commented on Sep 3, 2026

    @claude
    Contributor

    PM ACCEPT — PR #14827, head af8d0e7eb. Verified against the tree, not the report.

    domain:services execution seat, session session_01AUF1NoViznQK32gqpK8wS8. A resumption: the original dev was killed by the 03:44Z container restart with 128 lines uncommitted and a branch sitting at origin/main with zero commits. The resumed agent's first instruction was to commit and push before anything else — it did, and nothing was lost.

    Ruling of record — comments read: 1, and it IS the ruling: issuecomment-5504353873 (triage, 2026-09-02 04:24:49Z), read to its last line. That mattered: its closing half carries two fences, a pin instruction and a do-not-fold that the opening does not show.

    ⭐ The card is vindicated, and the discriminator is exactly the ruled one

    The ruling was emphatic that the fix must branch on whether the incoming signal already carries the child's output, and ⛔ not on a second loadRun lookup ("that reintroduces the same confusion one call over"). The diff adds carriesSubflowChildOutput(signal) and an else if before the existing else. No second lookup anywhere.

    ⭐ One thing better than what was asked. The predicate does not test the shape alone — it first requires the ENGINE_BUILT_SIGNAL symbol, and the docblock says why: output is a caller-writable field, and on this node the caller's signal is delegated down to the child, so a shape-only check would let a caller's own bag silence the genuine degraded warning. Only the engine can mint the symbol. A shape-only predicate would have passed every test in this card and left a real hole; catching that was not required by the ruling and is the best judgement call in the diff.

    The level fence — proved structurally, which beats an assurance

    I re-ran the dev's census myself at the merge base 5a5336b39 vs head:

    level base head
    warn 40 40
    error 6 6
    info 15 15
    debug 3 4

    And the decisive one: engine.ts has ZERO removed lines (git diff … | grep '^-' | wc -l = 0; the whole PR is +170/−1 and the single deletion is in the test file). You cannot change an existing line without removing it, so this structurally proves the degraded sentence, its level, and the else branch's parent continuation are untouched — a much stronger form of the claim than "I checked and did not change it". The one added debug line is exactly the option the ruling offered ("say nothing, or a debug line naming the carried output") and it names the carried output.

    Clause ② — no, verified independently

    I diffed the exported surface of engine.ts between base and head: IDENTICAL. carriesSubflowChildOutput is module-private. No new payload key, no accept/reject change. No carrier owed and none hung.

    Ablation

    Predicate forced to return false (restoring the old always-log behaviour): baseline Tests 10 passed (10) → ablated Tests 1 failed | 9 passed (10) with AssertionError: expected [ { level: 'warn', …(1) } ] to deeply equal [] — the original bug reproduced verbatim, which is the right failure to see — → restored Tests 10 passed (10).

    Mutation proved on disk by anchor count 1→0, injected globalThis marker 0→1, and blob 9f7ef89f5… → 059f24835…; restore proved by blob equality back to 9f7ef89f5…, marker count 0, empty git diff HEAD, clean git status. ⛔ No comment marker used. The no-rebuild condition is stated as measured, not assumed: the test imports '../engine.js', a same-package relative specifier vitest resolves to src/, and the red/green flip is itself the proof — an unread mutation would have stayed green.

    Package suite 101 passed (101) files / 1203 passed (1203) tests. Typecheck: 3 errors, none in either edited file, all three the TS2341 entries already recorded in check-type-check-coverage.mjs — debt moves by zero, and --listFiles confirms both edited files are in the checked set, so it is a measurement rather than a vacuous pass.

    Gates: 36 re-derived at the final commit, identical to the pre-changeset derivation (no new family; check-system-context-census passes — the failure mode that nearly bit a sibling card tonight). 33 exit 0; the other 3 exit 3 and say so in their own verdict text ("it is NOT a finding", "this is NOT a pass"). Exit codes captured by redirect-then-read, never across a pipe.

    Landing

    ⛔ Not landing yet. CI at af8d0e7eb is 23 success / 3 skipped / 5 still running, zero failures. Carriers clear on both card and PR. I land on converged gate conclusions, not on an aggregate read.


    Generated by Claude Code

  7. claude commented on Sep 3, 2026

    @claude
    Contributor

    Landed — PR #14827 merged at 2026-09-03T22:26:45Z (merge commit 06017edb3807); this card auto-closed at 22:26:46Z via its Fixes reference.

    pm:dispatched stripped in a read-modify-write with the read immediately before it, read back and diffed against the expected union — bug, domain:services, priority:p3 remain. No other label moved.

    Why this took 17 hours after it was accepted, recorded so the gap is not silent

    The work was PM-ACCEPTed at 04:29:36Z and then sat draft and unarmed until 21:59Z. The cause was a red Test Core (1/6), and the failure was not this PR's: packages/cli's run-dev-unbuilt-workspace.e2e.test.ts hit its 180000 ms ceiling (180104 ms, against a 7887 ms calibration of the same child on the same runner minutes earlier) while this diff only touches service-automation logging. That is the already-adjudicated defect #14832, under queue-flake anchor #14822.

    Its fix, #14875, landed on main at 11:34:02Z — about seven hours after this branch's head was pushed. So this PR's CI ran the test in exactly the window where the hang reproduces.

    The remedy was to bring the base in, not to touch the change: head af8d0e7eb7b4 → f6c6cb94abd2, after which Test Core (1/6) came back completed/success and the head was 30 success / 3 skipped / 0 failures of 33, read at job level. The only difference between the red run and the green one was the base — the cleanest available confirmation of the diagnosis.

    ⛔ No test skipped, disabled, or quarantined. ⛔ No re-run spent. ⛔ No empty commit, no close-and-reopen.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions