feat(runtime): report background task process and endpoint health

[liugddx]

Summary

Implements #5237 by making background-task readiness evidence explicit instead of treating process tracking as endpoint readiness.

• Persist and expose the native process PID after startup admission; provisional non-positive platform PIDs are omitted.
• Add the BackgroundTaskHealth tool with separate lifecycle/process and endpoint results.
• Endpoint checks require an explicit HTTP(S) URL, apply the existing target blocklist, use the policy-controlled transport, have a bounded deadline, and retry GET when HEAD returns 405/501.
• Report not_checked, checked/healthy, checked/unknown, or checked/unhealthy; no unverified ready claim is emitted.
• Logs are omitted by default and can be requested explicitly with include_logs; full output remains available through Read(ref).
• Keep privacy mode and missing network credentials fail-closed.
• New readers accept old records without pid; old binaries cannot read records written with the new field, which is the expected one-way storage upgrade for this additive field.
• Add focused runtime and host HTTP regression coverage for PID projection, metadata blocking, GET fallback, stalled endpoints, process-only reports, and failed probes.

Verification

• npm run build --workspace packages/core
• npm run build --workspace packages/runtime
• node --test packages/runtime/dist/__tests__/background-task-health-tool.test.js (3 passed)
• git diff --check

Relates to #5213

[Yx01-me]

Code Review: the overall direction looks right, but a few issues should be addressed before merge

The design (separating process tracking from endpoint readiness, failing closed, and reusing the policy-controlled transport) is sound. I verified each finding below against the main branch — all of them are introduced by this PR.

🔴 0. Formatting churn should be reverted; keep the diff minimal

In packages/core/src/shell-run.ts, the Pick<...> list for ShellRunPatch was previously a single compact line; this PR reformats it to one key per line. That region has no logic change — it's pure reformatting. It doesn't violate the biome check (I ran the repo-pinned Biome 2.5.11 format and lint over all 11 changed files: zero violations), but it's meaningless diff noise: it obscures the actual changes, increases review burden, and pollutes future git blame. Please restore the original formatting and only insert 'pid' into the existing list; likewise for the manual hard-wrapping in the new packages/runtime/README.md paragraph — match the style of the surrounding paragraphs instead.

🔴 1. probe() bypasses the cloud-metadata blocklist (security)

createHostWebFetchService's fetch() routes through createLocalWebFetchExecutor, which maintains a cloud-metadata host blocklist in runtime/src/local-web-fetch.ts (169.254.169.254, metadata.google.internal, etc.). The probe() added by this PR calls transport.fetch directly without going through that executor — so BackgroundTaskHealth can probe cloud metadata endpoints, which return host credentials. Being able to probe localhost is reasonable and intended, but the blocklist should apply equally. Suggestion: have probe() reuse createLocalWebFetchExecutor, or at minimum apply the same blocklist.

🔴 2. PTY mode can write pid: 0, breaking background task startup

The comment in pty-process-driver.ts explicitly notes that ConPTY publishes its inner PID some time after construction ("never cache the initial 0"). The guard in markRunning is live.driver.pid !== undefined — always true for this getter — so pid: 0 can be written. The new validation in normalizeShellRunRecord requires isPositiveInteger(record.pid), so updateShellRun → nextShellRunRecord throws, markRunning fails, and the entire background task startup is aborted. This is a timing-dependent bug. Suggestion: tighten the guard to live.driver.pid > 0 (treat invalid values as "not yet available"; pid is optional end to end, so absence is harmless).

🟡 3. Health result carries full logs by default, duplicating context

impl embeds the entire shell.output (stdout/stderr, or screen/scrollback for PTY with a 50KB budget) into the return value. The model can already get these logs via Read(ref). The underlying data is bounded by the truncation pipeline, and tool-runtime enforces a maxResultBytes gate (throwing ToolResultLimitError — failing the whole call — rather than truncating), but the default behavior still inflates context noticeably. Suggestion: omit logs by default, or gate them behind an include_logs parameter.

🟡 4. The three endpoint shapes are inconsistent; consider a discriminated union

not_checked → { status: 'not_checked' }
unknown     → { health: 'unknown', target, error }
healthy     → { status: 204, statusText, elapsedMs, target, health }

status is sometimes a string enum and sometimes a numeric HTTP status code, with no common discriminator. Suggestion: add a state discriminator ('not_checked' | 'checked') and carry the numeric code as httpStatus in the checked branch. Also, target currently reports only the origin — the probed path is lost. Separately, the README says a failed probe is unknown, the PR description says the tool reports not_checked/healthy/unhealthy, and the implementation actually has four states — the three descriptions should be aligned.

🟡 5. A 405 on the HEAD probe is reported as unhealthy, which is misleading

HEAD is optional in HTTP: servers that only registered GET routes (Go 1.22 method patterns, nginx limit_except, some gateways/WAFs) return 405/501 for HEAD while the service is actually healthy. The status < 400 verdict then produces an authoritative-looking unhealthy conclusion, and the model may restart a perfectly healthy service based on it. Suggestion: on 405/501, retry once with GET (discarding the body), and document the HEAD semantics in the tool description.

🟢 6. Cross-version compatibility should be declared

normalizeShellRunRecord validates records strictly via hasOnlyKeys(record, SHELL_RUN_RECORD_KEYS). Once a new version persists a record containing pid, older code will throw when reading it ('pid' is not in the old keys set). New-reads-old is fine; old-reads-new breaks. If this one-way upgrade is acceptable, please state it explicitly in the PR description.

[Yx01-me]

Overall direction looks right (separating process tracking from endpoint readiness, fail-closed on policy, reusing the strategy-controlled transport). Findings summarized in my top-level comment; all verified against main — each inline comment below links to the specific code.

[Yx01-me]

🔴 Formatting churn: please revert. This region has no logic change — the original single-line Pick<...> list was reformatted to one key per line. It passes biome (I ran the repo-pinned Biome 2.5.11 format/lint over all 11 changed files: zero violations), but it is meaningless diff noise: it obscures the actual change, burdens review, and pollutes future git blame. Please restore the original layout and just insert 'pid' into the existing list.

[Yx01-me]

🔴 Security: this raw transport.fetch bypasses the cloud-metadata blocklist. The fetch() path routes through createLocalWebFetchExecutor, which blocks cloud metadata hosts in runtime/src/local-web-fetch.ts (169.254.169.254, metadata.google.internal, ...). probe() calls transport.fetch directly, so BackgroundTaskHealth can probe metadata endpoints that return host credentials. Probing localhost is intended; the blocklist should apply equally. Suggestion: route the probe through createLocalWebFetchExecutor, or at minimum apply the same blocklist here.

Related: HEAD is optional in HTTP — servers that only registered GET routes (Go 1.22 method patterns, nginx limit_except, some gateways) return 405/501 for HEAD while the service is healthy. Consider retrying once with GET (discarding the body) on 405/501 before reporting a verdict.

[Yx01-me]

🔴 PTY mode can write pid: 0 here, which aborts startup. The comment in pty-process-driver.ts notes ConPTY publishes its inner PID some time after construction ("never cache the initial 0"), so this getter can legitimately return 0. pid !== undefined is always true for a getter, so pid: 0 gets persisted; the new validation in normalizeShellRunRecord requires isPositiveInteger(record.pid), so updateShellRun → nextShellRunRecord throws and markRunning fails the whole startup. Timing-dependent bug. Suggestion: tighten to live.driver.pid > 0 — treat invalid values as "not yet available" (pid is optional end to end, absence is harmless).

[Yx01-me]

🟡 Full logs are embedded by default, duplicating context. The model can already read these via Read(ref). The data is bounded by the truncation pipeline and tool-runtime's maxResultBytes gate (which throws ToolResultLimitError, failing the whole call, rather than truncating), but the default behavior inflates context noticeably — PTY scrollback alone has a 50KB budget. Suggestion: omit logs by default or gate them behind an include_logs parameter.

[Yx01-me]

🟡 The three endpoint shapes are inconsistent. status is a string enum here but a numeric HTTP status on the success branch, and the failure branch uses health with no status. Consider a discriminated union: { state: 'not_checked' } | { state: 'checked', httpStatus: number, health: 'healthy' | 'unhealthy' | 'unknown', target, ... } — one discriminator makes both model consumption and TS narrowing reliable. Also, target reports only the origin, so the probed path is lost. Separately, the README (failed probe → unknown), the PR description (not_checked/healthy/unhealthy), and the implementation (four states) disagree — please align all three.

[Yx01-me]

🟢 Cross-version compat to declare. normalizeShellRunRecord validates strictly via hasOnlyKeys(record, SHELL_RUN_RECORD_KEYS). Once a new version persists a record containing pid, older code throws on read ('pid' not in the old keys set). New-reads-old is fine; old-reads-new breaks. If this one-way upgrade is acceptable, please state it in the PR description.

[hqhq1025]

Reviewed commit e92b62a4143bb1d28a7b47ceef18ef54cab056b6. This change adds durable PID projection and the BackgroundTaskHealth process/endpoint report, but the production paths still have two PID failures and three endpoint-probe failures described inline, so the feature is not ready to merge.

Verification: build:test, full typecheck/lint/format, ASF headers, diff check, Core 831/831, Runtime 3466 passed / 13 skipped, focused changed paths 66 passed / 4 skipped, hosted test and audit, and a clean merge with current main 0d4a6ba5f. Runtime Host was 1916 passed / 19 skipped / 1 failed; the only failure was the unchanged managed-Bash sandbox integration because this runner rejects both unshare and bwrap.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

[P1] Preserve the PID when canonicalizing the record. This validator accepts record.pid, but canonicalShellRunRecord() rebuilds the object without that field. I ran a real ShellRunProcessManager with the SQLite-backed store and a live sleep 30; the initial background result, stored record, and later runtime-resource read all had pid === undefined while the task was running. As a result, the headline PID evidence never reaches BackgroundTaskHealth on the normal path. Please copy the optional PID in canonicalShellRunRecord() and cover the manager/store/read path rather than injecting a prebuilt result into the tool test.

[hqhq1025]

[P1] Do not persist ConPTY's provisional PID 0. PtyProcessDriver.pid explicitly documents that Windows ConPTY may expose 0 immediately after construction, but this condition includes it while normalizeShellRunRecord() requires a positive integer. With the production manager/store path and the driver PID held at 0, runBackgroundBash() failed with Invalid ShellRun record ... malformed fields and the durable task became failed before it could run. Treat non-positive values as unavailable (or wait for publication) and add a Windows/ConPTY regression.

[hqhq1025]

[P2] Apply the existing WebFetch target policy before sending this request. The normal fetch() path above goes through createLocalWebFetchExecutor, which rejects cloud-metadata targets; this new direct transport call bypasses that check. In a production-service probe, fetch("http://169.254.169.254/latest/meta-data/") was rejected before transport creation, while probe() issued the HEAD request and accepted a 204 response. The health tool should share the same metadata/target validation so it does not create a status-only SSRF path to addresses WebFetch deliberately blocks.

[hqhq1025]

[P2] A HEAD-only response is not sufficient to classify endpoint health. A local service whose registered route returned 200 to GET and 405 to HEAD was reported by the real BackgroundTaskHealth tool as health: "unhealthy", even though the endpoint was reachable and serving normally. Retry with a bounded GET (discarding or cancelling the body) for 405/501, or define a probe contract that distinguishes reachable/listening from application health.

[hqhq1025]

[P2] Give the health probe its own bounded deadline. This direct transport request only receives the turn's abort signal and bypasses the 30-second timeout used by the normal WebFetch executor. Against a real loopback server that accepted the TCP connection but never returned HTTP headers, probe() was still pending after 750 ms and settled only when I explicitly aborted the caller. An unresponsive endpoint can therefore pin the tool/turn indefinitely; compose a fixed timeout with caller cancellation and add a stalled-server regression.

[liugddx]

Addressed the substantive review findings in 0e45fac: preserve PID during canonicalization, ignore provisional ConPTY PID 0, apply the existing metadata target policy before probes, add a bounded probe deadline with caller cancellation, retry GET for 405/501, retain the full URL, use a consistent endpoint state/HTTP status shape, and omit logs by default (opt-in include_logs). Added real HTTP probe coverage for metadata blocking, GET fallback, and stalled responses. The formatting-only request is intentionally kept in Biome's required layout. The one-way old-reader compatibility behavior is documented in the PR description; new readers remain compatible with old records.

[hqhq1025]

Reviewed commit 0e45fac66c53a5c399f63c2aa9ad62dccd61fc6c. The Core projection and endpoint-probe findings are fixed, and provisional ConPTY PID 0 no longer fails startup. One Windows PTY persistence issue remains inline, so the PID-reporting feature is not ready across the supported paths.

Verification: build:test, lint, format check, ASF headers, diff check, focused Runtime 63 passed / 4 skipped, Runtime Host probe 6/6, and a real manager + SQLite + runtime Read probe with a ConPTY-shaped PID transition. Hosted audit is green; hosted test is still running. The branch is mergeable with current main 0d4a6ba5f. Native Windows execution was not available on this runner.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

[P2] Persist the PID when ConPTY publishes it after admission. On Windows, node-pty constructs the terminal with pid === 0 and updates it asynchronously after ready_datapipe. This guard correctly avoids rejecting the running transition, but markRunning() is the only place that adds pid; later output flushes and Read(ref) persist only output/status. With a real ShellRunProcessManager + SQLite store and a ConPTY-shaped driver whose PID changes from 0 to 4242 after startup, the driver reports 4242 while both the stored record and BackgroundTaskHealth/runtime Read still omit pid. Please add a later positive-PID persistence path (without regressing the startup guard) and a Windows/late-PID regression test; the current assertions explicitly allow undefined, so they cannot catch this.

[hqhq1025]

Reviewed commit 9d26eef0c0cebc1c2d3d6c673e15d2c8929611d8. The remaining late-published ConPTY PID issue is fixed, and I found no P0-P3 findings on the current head.

The manager now re-reads and persists a newly positive driver PID at every observation/flush/finalization cut. The new production-path tests hold the PTY PID at 0 through admission, then publish the real PID and verify both a later Read(ref) and an output-free exit persist it to SQLite and expose it through BackgroundTaskHealth; repeated reads do not create extra revisions. The previous Core projection and endpoint-probe fixes remain intact.

Verification: build:test, Core 823/823, Runtime 3338 passed / 13 skipped, focused Runtime 65 passed / 4 skipped, Runtime Host probe 6/6, lint, format check, ASF headers, diff check, hosted test and audit, and a clean merge with current main 0d4a6ba5f. Native Windows execution was not available on this runner; the late ConPTY transition was exercised by overriding the real PtyProcessDriver.pid getter around a real PTY process.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[jackwener]

Review — feat(runtime): report background task process and endpoint health

Reviewed SHA: 9d26eef0c0cebc1c2d3d6c673e15d2c8929611d8, verified as the live head immediately before publishing. Base d2e6c1f27, 14 files, +503/−7.

No P0–P2. One P3 below.

The thing I went looking for first

A tool that takes a URL from the model and fetches it is an SSRF question before it is anything else, and web-fetch-tool.ts growing by 48 lines alongside a new health tool is exactly the shape where a second, weaker network path gets introduced next to the hardened one.

That is not what happened here. probe calls the same assertAllowedTarget as WebFetch, resolves outbound execution through the same policy coordinator, and goes through the same proxied transport, so it grants the model no reach that WebFetch did not already grant. Privacy mode and a missing proxy credential both refuse before any connection. redirect: 'manual' means a redirect cannot be used to step past the check, and the body is cancelled on both the HEAD and the GET fallback.

The guard is pinned rather than merely present: removing assertAllowedTarget(parsed) turns exactly one test red — health probes reject metadata before creating a transport — and that test asserts the ordering too, so a future refactor cannot satisfy it by rejecting after opening a connection.

Result semantics

The two-axis shape is honest about what it proves. With no url the endpoint is not_checked rather than assumed; a probe failure yields unknown with the target echoed; and because redirects are not followed, a 3xx maps to unknown rather than healthy. The tool description states plainly that it reports HTTP status only, not browser loading or ownership of the listener — which is the right disclaimer for a check that a wrong process listening on the same port would also pass.

Registration is consistent: categoryHint: 'web_read', the same category as WebFetch and WebSearch, and it joins childHostTools so a child agent gets it under the same permission handling rather than a bespoke one.

pid plumbing

pid is threaded through every place the schema enumerates fields — ShellRunRecord, ShellRunPatch, SHELL_RUN_PATCH_KEYS, SHELL_RUN_RECORD_KEYS, the result metadata and the shape's optional list — and validated as a positive integer in normalizeShellRunRecord. Adding a field to a hand-maintained key set is a common place to miss one entry; none is missing here.

P3 — a failed probe's message reaches the model without the redaction applied to thrown tool errors

When the probe throws, the tool catches it and embeds error.message verbatim in a successful result. Errors that propagate out of a tool instead go through formatSyntheticToolErrorText, which applies redactSecrets. This path bypasses that.

I did not establish that any secret can actually reach it. The proxy credential is carried as a structured field rather than embedded in a URL, and the transport's own errors are generic, so I could not construct a leak. I am reporting the asymmetry rather than a demonstrated exposure: a defence that exists for one route out of a tool is absent on another route that carries provider- and proxy-originated text.

Minimal change if you want it closed: run the caught message through the same redaction before embedding it, or return a fixed reason with the detail kept out of the model-visible result.

Smaller note, not a finding

execution-composition.ts:658 uses runtimeResources!. The variable is assigned unconditionally earlier and the same object is used without assertion at several nearby lines, so this reads as satisfying narrowing rather than an unchecked risk. Mentioned only so it is a deliberate choice rather than an unnoticed one.

Verification basis and limits

Built @maka/core, @maka/storage, @maka/runtime and @maka/runtime-host at this SHA. background-task-health-tool and shell-run-manager run 64 passed / 4 skipped / 0 failed; web-fetch-tool 6 passed; plus the ablation above, on artifacts confirmed to contain the change before running.

I did not run the full suites, and I did not exercise a real background process or a real endpoint — the probe behaviour I checked is at the transport-policy level, not against a live listener. My conclusions are independent of CI status.

---

Automated review, agent-operated. Posted from the shared jackwener GitHub account; the reviewing agent is @kabi-opus (human owner: 卡比卡比 / @WAWQAQ), acting at @liugddx's request. AI-assisted review; does not replace independent human review. No merge is performed and none is authorised by this review.

[Astro-Han]

Thanks for following through on the earlier findings. I reviewed 9d26eef0c0cebc1c2d3d6c673e15d2c8929611d8 and found no actionable P0–P3 issues.

The #5237 distinction is useful: a tracked/running process is not evidence that an HTTP endpoint is ready. This change keeps ShellRun as the process authority and reuses the existing Host network policy, target guard, and transport for the optional probe. It does not introduce another process manager or network-policy implementation.

The earlier substantive findings are addressed: PID survives canonicalization; provisional zero is omitted; a later positive PID is captured at observation/finalization without creating revisions on unchanged reads. Endpoint probes apply the metadata guard before creating a transport, preserve privacy/proxy policy, have a bounded deadline and caller cancellation, and retry GET once for HEAD 405/501. Logs remain opt-in, and failed or omitted probes do not claim readiness.

Validation: I ran the three health-tool tests and six Host web-fetch tests against the changed source, including the real loopback HEAD/GET and stalled-response cases; all nine passed. I inspected the manager-to-SQLite late-PID regression paths but did not rerun the full manager suite or test native Windows ConPTY. Current CI test/audit checks pass. The error-message redaction asymmetry raised earlier has no demonstrated exposure in this review, so I am not treating it as a blocker.

AI-assisted review with Codex and Reviewer Sol; I cross-checked the findings and verification above.