Document the child_process spawn contract for components - #634
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the documentation for Harper's child process spawning mechanism, adding a detailed section on child processes in the JavaScript environment reference, updating configuration options to reflect the new default vm-current-context module loader, and revising the v5 migration guide. The review feedback highlights a few documentation improvements, including correcting the version badge format in the environment reference, fixing a typo in the configuration options, and addressing grammatical errors in the migration guide.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-634 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-634 This preview will update automatically when you push new commits. |
Documents the
child_processsubstitution contract that Harper applies to component code: which loaders and import forms receive the constrained module, which of Node's functions are usable, howapplications.allowedSpawnCommandsmatching actually works, the mandatorynamePID-file lock at<rootPath>/pids/<name>.pid, the integer-onlyversionreplacement semantics, and the narrowExistingProcessWrapperthat every caller except the lock winner receives. Closes #626.Also corrects two adjacent errors found while verifying the contract against core:
applications.moduleLoader's documented default wasvm(it isvm-current-context), and bothreference/configuration/options.mdand the v5 migration guide listedexecas an allowlisted spawn function whenexeccannot be called through the substitute at all.For the human reviewer
The page documents defects, not just contract. Verifying the issue's claims against
security/jsLoader.tsturned up several behaviors that are bugs rather than design, and I chose to describe them rather than write the contract as intended and leave readers to discover them:execis unreachable. The substitute forwards(command, args, options, callback)positionally into Node'sexec(command[, options][, callback]), so the idiomatic two-argument call fails the mandatory-namecheck and the shifted form throwsERR_INVALID_ARG_TYPE(confirmed empirically against Node, not just read).versionreplacement path can leave two processes running: the outgoing child'sexithandler unlinks the PID file by path rather than by PID, so it can delete the lock the replacement just wrote. The page steers readers away from in-place replacement because of this.unref()on the wrapper clears the only liveness interval, which is also the only source of its'exit'event — so the two members are mutually exclusive.nameis interpolated into the lock path unsanitized and is not namespaced per component.If the team would rather fix these in core than publish them, the corresponding paragraphs should come out; that is the main judgment call here. Issue #626 also offered a second option — declare the contract internal and point components at a supported once-per-node primitive. I took the "document it" branch because the gap is live today either way, but that is a product call, not a docs call.
Scope decisions worth a second opinion. The section lives inside
reference/components/javascript-environment.mdrather than a dedicated page (it is now ~95 lines on a page otherwise about globals). It sanctionsif (child.spawnargs)as the way to tell a realChildProcessfrom the wrapper, which is a duck-typed discriminator core never promised. And theversionoption carries<VersionBadge version="v5.0.2" />— a patch version, which deviates from AGENTS.md'svX.Y.0convention, but the option genuinely shipped in a patch and rounding it tov5.0.0would be wrong.Not addressed here: the core defects above are not filed as issues yet, and
applications.lockdown's documented default (freeze) also disagrees with the migration guide (freeze-after-load) — left alone as unrelated to this change.Verification
Every behavioral claim was traced to
harpersecurity/jsLoader.tsandcomponents/ApplicationScope.tsonorigin/main(v5.2.4), and theexecfailure was reproduced directly against Node 24 rather than inferred.npm run buildis clean — the two broken anchors it reports are pre-existing onmain(backups/overview,release-notes/v5-lincoln/5.1) and unrelated.npx prettier --checkis clean on the changed files. The rendered page was inspected inbuild/to confirm the new tables and anchors resolve.Complexity: medium
Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=6 @ c0883b7
Human-Review-Need: 4 @ c0883b7