Repository navigation
INTR-719 - Remove the retired agent code - #256
Conversation
Every deployment wrote the same empty deploy content, and nothing read the column back.
Nothing in production stages a step's deploy tree any more: a deployment carries its definition and tools in its source closure. Sidecars stopped pushing state packs too, and the Hub rejected them anyway. writeTree stops clearing a prefix, since only these paths asked it to, and createAsset stops refusing a package-registry name that matches an HTTP registry, since the launch path that rule protected is gone.
It only verified the signatures of deploy packs, and the Hub no longer sends any. An older sidecar still requires the key on the deploy frame, so a Hub and its sidecars have to be upgraded together.
alexanderguy
left a comment
alexanderguy
left a comment
There was a problem hiding this comment.
This is an agentic review that used the pull-request-review skill. I checked the findings below against the branch myself.
The removal is thorough. I found no defects in correctness, security, wire compatibility or the migration. I request changes for two items: a module that the PR leaves with no callers, and some comments that still describe removed behaviour.
What I checked
make allexited 0 on147c117d0. The database suites skipped there because the worktree had no.envfiles.- With the
.envfiles present,make testexited 0 with 0 failures (10319, 255 and 1511 tests passed). tests/workflow-deploy/asset-tool-restarts.e2e.test.ts, run alone: 1 test passed.- Migration 0118 drops only
deploy_content. The journal entry, the snapshot and the Drizzle schema agree. - All commit subjects and bodies are within 72 characters, with no prefixes or references to trackers or other commits.
1. The pinned tool-package materializer has no callers
9467639 deletes apps/sidecar/src/step-tool-materialization.ts. That file was the only production caller of materializeToolPackages. At HEAD, packages/tool-packaging/src/tool-materialization.ts (571 lines) and its 349-line test file stay in the tree. Only the barrel export at packages/tool-packaging/src/index.ts:96-98 and comments refer to them. This applies to materializeToolPackages, parseActiveDeployId, clearDirtyMarker, persistActiveDeployIdWithFallback and StepToolFactory.
Could we delete the module, its tests and the barrel exports in 9467639? That commit says it removes this runtime, so the deletion fits there.
These comments name the module or StepToolFactory and need an update when it goes:
packages/tool-packaging/src/index.ts:26packages/tool-packaging/src/workflow-closure-apply.ts:12packages/workflow-host/src/workflow-definition-loader.ts:15packages/harness/src/step-credential-capabilities.ts:85tests/workflow-deploy/single-step-credential-tool.test.ts:19tests/workflow-deploy/fixtures/credential-tool-workflow.ts:12
@intx/tool-packaging has publishConfig.access: public, so removing these exports breaks its public API. The PR also removes exports from other public packages (hub-agent, storage-isogit, workflow-host and workflow-deploy). Could the description list these API breaks in the Upgrade section?
2. Comments still describe per-step mail addresses
8c07a3c and 097e925 remove per-step address derivation (deriveStepAddress). At HEAD, no code builds a <runId>-<stepId>@... address. These comments still give that derivation as the reason for a rule:
packages/workflow/src/definition/workflow.ts:150says a run id is the local part of a derived per-step mail address. The path-segment reason in the same comment is still correct.packages/workflow/src/definition/workflow.ts:473-479says the orchestrator derives per-step mail addresses.packages/workflow-host/src/deploy/workflow-host-wiring.ts:562-564saysSTEP_ID_PATTERNexists so per-step mail-address derivation needs no escaping.tests/workflow-deploy/boot-restore.bench.ts:403-404says the setup puts step grants on disk. The bench sends norun.grantsframe, so it writes no grants. The file header already has the corrected text.
The ID pattern checks are still correct for path segments. Only the stated reasons need an update.
The LAYOUT.md item has an inline comment.
| * the data. v1 writes them directly to disk; the formal git-commit | ||
| * step (turning these into a signed audit-trail commit on the | ||
| * agent-state repo) is a separate plumbing piece. | ||
| * step (turning these into a signed audit-trail commit) is a separate |
There was a problem hiding this comment.
This comment changes, but nothing calls this module after 9467639. That commit deletes apps/sidecar/src/step-tool-materialization.ts, which was the only production caller of materializeToolPackages. Could we delete this file, its test file and the exports at index.ts:96-98 in 9467639? The comment at lines 55-58 also says the child reaches this function through the substrate factory's per-step agent builder, and that path no longer exists.
There was a problem hiding this comment.
Keeping this for now. It goes with tool package pins in a follow-up PR. I fixed the stale comment at lines 55-58.
| | `@intx/hub-agent` | Sidecar-side orchestrator (`createSidecarOrchestrator`, the core seam `apps/sidecar` wires together). Wires the package-side pieces of the sidecar runtime — the per-agent key and repo stores, a session manager, and the hub WebSocket link — into a single start/close handle and forwards a spawned child's verified inference events back to the hub. In-process harness construction and agent provisioning are retired: every agent runs as a supervised workflow-process child on the workflow-run substrate (see `@intx/workflow-host`). | | ||
| | `@intx/tool-packaging` | Hub-side closure resolver and sidecar-side loader for the npm-distributed tool-package format. Resolves a manifest's dependency closure, materializes tarballs into a content-addressable cache, and lays out a per-instance nested `node_modules/` so a tool package and its transitives resolve bare-specifier imports against the manifest closure. Used by both `apps/hub` and `apps/sidecar`. | | ||
| | `@intx/workflow-host` | Supervisor and child orchestration for running agents as supervised workflow-process children: the dispatch loop, inbox claim-check, the `SubprocessSpawner`/`SubprocessHandle` spawn seam, the per-step step-invoker, and recycle/drain. It also holds the deploy program, the child substrate factory, the probe protocol, durable conversation state, and the per-run grants reader. Consumes the `@intx/hub-sessions/substrate` repo primitives and takes its spawner, mail bus, and step invoker as injected bindings. Intended to stay host-agnostic so an alternate host reuses it with a different tool/agent runtime — see the open item under Dependency Rules. Its `./local` subpath (the step invoker, step tool builders, authorize closure, and local credentials snapshot) imports no Node or Bun module and bundles for a browser, so `runLocal` callers run definitions outside a sidecar. | | ||
| | `@intx/workflow-deploy` | Deploy-time validation and derivation over the workflow-run substrate: the capability walk and operator-approval gate (`walkCapabilities`, `createApprovalSetGate`), the child grant cap (`collectDeclaredResources`, `filterGrantsToDeclaredResources`), the runtime grant projections of the walk (`runtimeGrantRules`, `credentialUseGrantRules`), pure deployment-address and workflow-run repo-id derivation (`deriveRunAddress`, `deriveStepAddress`, `deriveWorkflowRunRepoId`), and per-step inference-source pinning (`pickStepInferenceSource`). An agent-runtime package (it depends on `@intx/agent`, `@intx/authz`, and `@intx/workflow`) consumed by `apps/sidecar`, `@intx/hub-sessions`, and `@intx/workflow-host` (its `./local` entry) on the host side and reached into by `@intx/hub-api` on the control-plane side — one of the enumerated crossings in the Dependency Rules. | |
There was a problem hiding this comment.
This row still lists deriveStepAddress, but 097e925 removes it from @intx/workflow-deploy. Could we drop it from the list so the row matches the package exports?
Source-ref is the only deploy lineage, so a step's tools always come from the agent's evaluated factories, and nothing stages the deploy tree the pinned arm read.
The Hub receives no state packs and writes no deploy commits any more, so those repos have nothing left to collect.
The state.git route only served state written by flows that no longer exist, and the per-definition route it was documented beside is already gone. The asset service has always refused to create an agent-state asset, so no asset row can carry that kind. The timeline reconstruction that read these repos and the mail audit store only it read had no other callers, so they go too.
The Hub always sent an empty grant list on the deploy frame, and a run's authority comes from its run.grants frames and grants file. The per-step grant repos and the spawn-time snapshot built from them did nothing, except that a spawn could fail over grants nothing used. A step's derived mail address goes too, since only that snapshot still carried it and nothing read it there. An older sidecar still requires config.grants on the deploy frame, so a Hub and its sidecars have to be upgraded together.
Nothing writes or reads these repos any more, so the kind, its handler and its authorization go. A leftover agents directory on a Hub or a sidecar is safe to delete.
Nothing configures it any more, and a prepared receive refuses any kind it reclaims, so it could not serve workflow-run repos as it stands.
Nothing calls it any more. A workflow-run receive goes through stagePack, and a source asset pack through indexPackIntoGitDir.
Nothing reads a launch error's phase or the key store's in-memory map, and undeploying an agent only cleared that map.
Nothing configures it any more: neither the Hub nor the sidecar passes a policy, so only tests run it. A workflow-run repo has to keep its history anyway, because an expunged mail's bytes persist only through older commits. With nothing pruning, a missing parent is lost data, so checkpoint and transform record reads now throw on one instead of returning what survives.
Nothing reads them: the sidecar takes only the config's session id, and it hosts nothing but workflows. The frame's config shrinks to that id, its agent id goes and its workflow is required. The Hub keeps the rest of the harness config to build its pins.
Every spawn carries the directory, so the branches for it being absent could never run.
The router is their only reader, so they no longer travel through the child's spawn env as strings to be parsed back.
Nothing ever dereferenced it, and the audit store it pointed at does not exist. Entries already on disk carry the field and still parse. An older Hub still requires the field on a pushed entry, so a Hub and its sidecars have to be upgraded together.
Nothing ran a real tool from an asset mount across process deaths.
The Hub computed a wire hash for each referenced body and sent it on the deploy frame, but the sidecar never read it. A body is resolved in memory from the parent's closure, which the child has already verified.
147c117 to
1dcf77e
Compare
alexanderguy
left a comment
alexanderguy
left a comment
There was a problem hiding this comment.
This is an agentic re-review that used the pull-request-review skill.
The new version fixes the stale comments from the first review. Only comments and LAYOUT.md changed. The other 16 commits have the same patches as before.
On 1dcf77ec9, make all with the database env present exited 0. Lint, the type check and vite build passed, and the tests had 0 failures.
Keeping the materializer for the follow-up is fine. I created INTR-725 to track removing the tool package pins and the materializer, and assigned it to you.
Summary
Everything runs as a workflow now, so this removes the
agent-staterepo kind, itsstate.gitroutes, its GC settings and the deploy-tree path, along with the code only they reached. Nothing in production stages a deploy tree or writes these repos any more.What changes
agent-stateis gone from the repo kinds, asset APIs and authorization, and bothstate.gitroutes are gone. Nothing usesagents/now, so a leftover one is safe to delete.stageWorkflowStep,executeLaunchPhases,writeDeployTree, the provision-step frame and the state-pack receive path are gone. A step's tools always come from its agent's evaluated factories.HUB_AGENT_GC_*settings and the collector are gone. With nothing pruning, a missing parent is lost data, so checkpoint and transform record reads throw on one.applyPack, the deployment grants path and several deploy frame fields are gone.asset-tool-restartsruns a real tool from a mounted asset across a child crash and a sidecar restart.Upgrade
Upgrade a Hub and its sidecars together: stop the Hub, run migration
0118(drops the unusedworkflow_run_launch_spec.deploy_content), then start both. An old Hub rejects a new sidecar's mail entries, which no longer carrymailAuditRef, and an old sidecar rejects a new Hub's deploy frames, which no longer carryhubPublicKey.Not in this PR
main.session_assettable stays, though this removes its only writer.bin/devbuiltins publish stays.