feat(modules): engine-as-deployer foundation (HT-119) - #187
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change adds a module marketplace CLI, signed artifact validation, safe extraction, Vercel and local deployment providers, encrypted installation persistence, endpoint possession checks, and a queued installer with leases, retries, cleanup, and audit events. ChangesModule installation and deployment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Queue
participant ModuleInstallStore
participant Marketplace
participant DeployProvider
participant WebhookEndpoint
Queue->>ModuleInstallStore: Claim module install lease
ModuleInstallStore-->>Queue: Return fenced install state
Queue->>Marketplace: Download verified artifact
Marketplace-->>Queue: Return verified tarball and manifest
Queue->>DeployProvider: Create project and upload artifact
DeployProvider-->>Queue: Return deployment identifiers
Queue->>DeployProvider: Create deployment and poll state
DeployProvider-->>Queue: Return ready deployment URL
Queue->>WebhookEndpoint: Verify endpoint possession
WebhookEndpoint-->>Queue: Return matching HMAC
Queue->>ModuleInstallStore: Persist active state and audit event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (20)
src/modules/deploy/vercel-adapter.ts (1)
302-321: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the body stream when the cap is exceeded.
readBoundedTextthrows while the reader still has an unread remainder. Thefinallyblock releases the lock but never cancels the stream. The remaining bytes stay pending until the socket is collected. Callreader.cancel()on the throw path so the connection is torn down at once.♻️ Proposed refactor
const reader = response.body.getReader() const chunks: Uint8Array[] = [] let total = 0 + let capExceeded = false try { for (;;) { const { done, value } = await reader.read() if (done) break if (!value) continue total += value.byteLength if (total > capBytes) { + capExceeded = true throw new VercelAdapterError( `vercel-adapter: refusing response — body exceeded the ${capBytes}-byte cap while streaming`, ) } chunks.push(value) } } finally { + if (capExceeded) await reader.cancel().catch(() => {}) reader.releaseLock() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/deploy/vercel-adapter.ts` around lines 302 - 321, Update readBoundedText’s cap-exceeded path to cancel the body reader before propagating VercelAdapterError. Ensure reader.cancel() runs when total exceeds capBytes, while retaining the existing releaseLock cleanup and normal streaming behavior.src/store/module-license.test.ts (1)
14-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack the inline DDL so it cannot drift from the future migration.
freshDbduplicates themodule_licenseschema. The store's SQL depends on the exact column set. If the migration lands with any difference, these tests still pass against the stale local copy. SwitchfreshDbtomigrate(db)as soon as the migration exists.Do you want me to open an issue to track that switch?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/module-license.test.ts` around lines 14 - 35, Update freshDb to call the existing migrate(db) helper instead of maintaining inline CREATE TABLE DDL, once the module_license migration is available. Remove the duplicated schema definition while preserving creation of a fresh PGlite database and returning the initialized Db instance.src/modules/deploy/vercel-adapter.test.ts (1)
151-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this block to match what it asserts.
The describe block is named
allowlist, but the test assertsassertSafeIdrejection of an emptydeploymentId. No allowlist miss occurs. Rename the block and the test so a future reader does not treat this as allowlist coverage. Add a separate case that reachesassertAllowlisteddirectly if you want that coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/deploy/vercel-adapter.test.ts` around lines 151 - 171, Rename the `describe('createVercelDeployProvider — allowlist')` block and its test to reflect that they cover `assertSafeId` rejection of an empty `deploymentId`, not allowlist enforcement. Do not label this case as allowlist coverage; add a separate direct `assertAllowlisted` case only if allowlist behavior must also be tested.src/store/module-license.ts (1)
233-244: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize
updated_atbefore returning it.
ModuleLicenseRow.updated_atis typed asstringbut maps fromtimestamptz. Since other stores acceptDate | stringfor the row value and convert it withtoDate, makeModuleLicenseRow.updated_at/LicenseDisplayInfo.updatedAtaccept both inputs and normalize before returningLicenseDisplayInfo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/module-license.ts` around lines 233 - 244, Update ModuleLicenseRow.updated_at and LicenseDisplayInfo.updatedAt to accept Date | string, then normalize row.updated_at through the existing toDate utility in getDisplayInfo before constructing the returned display object.tests/modules/artifact/vectors.test.ts (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the unknown-key fixture without
replace, and silence the CodeQL alert.CodeQL reports incomplete string replacement here. The alert is a false positive for this fixture, because
MANIFEST_JSONhas no nested object and therefore exactly one{. The construction is still fragile against a future vector with a nested object. Build the JSON from the parsed manifest instead.♻️ Proposed change
it('rejects a manifest with an added unknown key', () => { - const withExtra = MANIFEST_JSON.replace('{', '{"unexpected":"field",') + const withExtra = JSON.stringify({ ...JSON.parse(MANIFEST_JSON), unexpected: 'field' }) expect(() => parseManifest(withExtra)).toThrow(/unknown field/) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/modules/artifact/vectors.test.ts` around lines 78 - 81, Update the unknown-key fixture in the test around parseManifest to avoid string replacement: parse MANIFEST_JSON, add an unexpected field to the resulting manifest object, and serialize it back to JSON before calling parseManifest. Preserve the existing assertion that the parser rejects the added unknown key.Source: Linters/SAST tools
tests/modules/artifact/extract.test.ts (2)
26-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the symlinked-destination refusal.
safeExtractrefuses adestDirthat is itself a symlink, because the containment checks reason about the link path. That control has no test here. Add one so a later change fromlstatSynctostatSyncfails the suite.💚 Proposed test
it('refuses a non-empty destination', async () => { fs.writeFileSync(path.join(destDir, 'already-here.txt'), 'x') await expect(safeExtract(buildTarGz([regularFile('a.txt', 'hi')]), destDir)).rejects.toThrow( /not empty/, ) }) + + it('refuses a destination that is a symlink', async () => { + const real = fs.mkdtempSync(path.join(os.tmpdir(), 'ht116-extract-real-')) + const link = path.join(destDir, 'link') + fs.symlinkSync(real, link) + try { + await expect(safeExtract(buildTarGz([regularFile('a.txt', 'hi')]), link)).rejects.toThrow( + /is a symlink/, + ) + } finally { + fs.rmSync(real, { recursive: true, force: true }) + } + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/modules/artifact/extract.test.ts` around lines 26 - 39, Add a test in the “safeExtract: destination preconditions” suite that creates a symlink as the destination path and verifies safeExtract rejects it, asserting the expected refusal error. Use the existing destDir and temporary path setup without altering the current missing and non-empty destination tests.
130-159: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffEnsure CI can run the artifact caps under the current memory budget.
runes-on: ubuntu-latesthas no explicit job-level memory reservation for theseBuffer.alloccap tests; one cap allocates 128 MiB and the archive cap allocates 512 MiB beforebuildTarGz()copies them into a tar buffer. If CI is constrained, replace the buffer bodies with sparse data and add asizeOverride/paxSizeOverrideheader so the cap still rejects over-cap archives without forcing the allocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/modules/artifact/extract.test.ts` around lines 130 - 159, Reduce memory usage in the artifact cap tests around the oversized entries and archive-cap case by using sparse/minimal file bodies while preserving their declared sizes through the existing tar-entry size override mechanism, such as sizeOverride/paxSizeOverride. Ensure buildTarGz still produces headers declaring MAX_SINGLE_FILE_BYTES or the over-cap total so safeExtract rejects for the same size-limit reasons without allocating 128–512 MiB buffers.tests/modules/artifact/module-config.test.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the "byte-for-byte" claim.
The fixture is built by
JSON.stringifyover an object literal, so it reproduces the shipped configuration's values, not its bytes. Whitespace and key order in the released file are not asserted anywhere. State that this mirrors the shipped configuration's contents.♻️ Proposed change
-/** The REAL module.config.json shipped in the draft-assistant 0.3.0 release artifact, byte-for-byte. */ +/** Mirrors the contents of the module.config.json shipped in the draft-assistant 0.3.0 release artifact. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/modules/artifact/module-config.test.ts` around lines 8 - 9, Update the comment above REAL_DRAFT_ASSISTANT_CONFIG to remove the “byte-for-byte” claim and state that the fixture mirrors the shipped configuration’s contents or values, without implying preservation of whitespace or key order.src/modules/catalog/marketplace-client.test.ts (1)
156-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the signature and trust-store refusals.
The suite covers identity binding, the digest, the size cap, and license-key redaction. It does not cover
bad-signatureoruntrusted-key, which are the two controls that make the pinned key meaningful. ThetrustStoreseam andmakeTestKeypairalready make both cases cheap: sign a manifest with a second keypair while the catalog reports the trustedkeyIdto reachbad-signature, and omit thekeyIdfromtrustStoreto reachuntrusted-key.served-version-mismatchandmanifest-parse-errorare also uncovered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/catalog/marketplace-client.test.ts` around lines 156 - 198, Expand the downloadVerifiedArtifact test suite with refusal cases for bad-signature and untrusted-key, using setup, makeTestKeypair, and trustStore: sign the manifest with a second keypair while retaining the catalog’s trusted keyId for bad-signature, and remove the keyId from trustStore for untrusted-key. Also add coverage for served-version-mismatch and manifest-parse-error, asserting each result reports its corresponding refusal code.src/modules/artifact/extract.ts (1)
296-300: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueWrap write-phase failures so every refusal stays an
UnsafeArchiveError.The validation loop does not detect a file entry that another entry later uses as a directory prefix. An archive containing
aas a regular file and thena/bpasses every check above, because the duplicate keyadiffers froma/b.fs.mkdirSyncthen throws a rawENOTDIR, not the named refusal this module promises, and the loop has already writtena. The comment on Lines 206-208 only covers refusals during validation.Reject the conflict during validation, and translate unexpected write errors into
UnsafeArchiveError.♻️ Proposed change
for (const { fullPath, data } of toWrite) { - fs.mkdirSync(path.dirname(fullPath), { recursive: true }) - fs.writeFileSync(fullPath, data) - written.push(fullPath) + try { + fs.mkdirSync(path.dirname(fullPath), { recursive: true }) + fs.writeFileSync(fullPath, data) + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + throw new UnsafeArchiveError( + `archive entry could not be written (conflicting file/directory entry?): '${path.relative(resolvedDest, fullPath)}': ${message}`, + ) + } + written.push(fullPath) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/artifact/extract.ts` around lines 296 - 300, Update the validation loop in extract.ts to reject any archive entry whose path is a descendant of an earlier regular-file entry, including conflicts such as “a” followed by “a/b”; preserve the existing UnsafeArchiveError refusal behavior. Also wrap the write phase around the loop over toWrite, including mkdirSync, writeFileSync, and written updates, so unexpected filesystem failures are caught and rethrown as UnsafeArchiveError rather than leaking raw errors or leaving partial writes unreported.tsconfig.json (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnforce the
src/→cli/import boundary.
src/modules/catalog/marketplace-client.tsdocuments thatcli/is a separate workspace and this engine must not import it, buttsconfig.jsonincludescli/src/**/*.tsin the same root program and there is no lint rule for this boundary. Add a lint boundary rule, or givecli/its owntsconfig.jsonand reference it as a project instead of merging its sources into the root program.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tsconfig.json` at line 14, Update the TypeScript project configuration around the root include entry so cli/src is no longer merged into the engine’s root program. Either add an explicit lint rule preventing src modules such as marketplace-client from importing cli code, or give cli its own tsconfig.json and reference it as a separate project while preserving independent compilation.cli/src/install.ts (1)
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the
getLicenseKeydoc comment.The comment states that the real wiring of
getLicenseKeychecks$HELPTHREAD_LICENSE_KEYfirst.cli/src/main.tswiresgetLicenseKeytopromptHiddenalone, and Line 78 performs the environment-variable check. Update the comment so the responsibility is described once and in the correct place.♻️ Proposed comment fix
- /** Returns the license key. Real wiring checks $HELPTHREAD_LICENSE_KEY first, then prompts hidden. */ + /** Prompts for the license key. `runInstall` reads $HELPTHREAD_LICENSE_KEY first and only calls this when that variable is unset. */ getLicenseKey: () => Promise<string>Also applies to: 78-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/install.ts` around lines 32 - 33, Update the getLicenseKey doc comment in the install interface to describe only its license-key retrieval behavior, removing the claim that it checks HELPTHREAD_LICENSE_KEY first. Keep the environment-variable check documented at the existing line 78 location where that responsibility is implemented.tests/cli/env-summary.test.ts (1)
62-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the
(none)test.The test name says "an empty side", but only the operator-managed side is covered. Add the engine-managed case. Also select the fixture entry by name instead of by index, so a reordering of
CONFIG.envdoes not break this test for an unrelated reason.♻️ Proposed test change
- it('renders "(none)" for an empty side', () => { - const noOperator: ModuleConfigV1 = { - schemaVersion: 1, - module: 'engine-only', - env: [CONFIG.env[0]], - } - const out = renderEnvSummary(noOperator) - const youSupplyIndex = out.indexOf('You supply these:') - const engineIndex = out.indexOf('Your Helpthread engine mints these') - expect(out.slice(youSupplyIndex, engineIndex)).toContain('(none)') - }) + const byName = (name: string) => { + const found = CONFIG.env.find((v) => v.name === name) + if (!found) throw new Error(`fixture is missing ${name}`) + return found + } + + it('renders "(none)" for an empty operator-managed side', () => { + const out = renderEnvSummary({ + schemaVersion: 1, + module: 'engine-only', + env: [byName('HELPDESK_API_URL')], + }) + const youSupplyIndex = out.indexOf('You supply these:') + const engineIndex = out.indexOf('Your Helpthread engine mints these') + expect(out.slice(youSupplyIndex, engineIndex)).toContain('(none)') + }) + + it('renders "(none)" for an empty engine-managed side', () => { + const out = renderEnvSummary({ + schemaVersion: 1, + module: 'operator-only', + env: [byName('ANTHROPIC_API_KEY')], + }) + const engineIndex = out.indexOf('Your Helpthread engine mints these') + expect(out.slice(engineIndex)).toContain('(none)') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/env-summary.test.ts` around lines 62 - 72, Strengthen the test around renderEnvSummary by selecting the operator environment fixture from CONFIG.env by its identifying name rather than index, then add coverage for the engine-managed empty side as well as the existing operator-managed empty side. Preserve the "(none)" assertions for both summary sections.cli/src/main.ts (1)
93-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a friendlier message for unexpected errors.
Only
InstallErrorandVerifyCommandErrorbecome clean messages. A network failure insidefetchCatalogorfetchTarballsurfaces as aTypeError, so the operator sees a raw stack trace. Print a short message first and keep the stack behind an environment flag such asHELPTHREAD_DEBUG.♻️ Proposed refactor
(err) => { - console.error(err instanceof Error ? (err.stack ?? err.message) : String(err)) + const message = err instanceof Error ? err.message : String(err) + console.error(`Error: ${message}`) + if (process.env.HELPTHREAD_DEBUG && err instanceof Error && err.stack) { + console.error(err.stack) + } process.exitCode = 1 },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/main.ts` around lines 93 - 101, Update the rejection handler for main() to print a concise user-facing message for unexpected errors before any stack trace. Preserve clean messages for InstallError and VerifyCommandError, and only include the stack when the HELPTHREAD_DEBUG environment flag is enabled; otherwise avoid exposing raw TypeError stack output from fetchCatalog or fetchTarball.src/modules/artifact/module-config.ts (1)
106-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating the shape of
name.
requireNonEmptyStringaccepts any non-empty string forenv[i].name. Values such asA=B, a name with a newline, or a leading digit pass validation. Downstream consumers set these names as deployment environment variables, so an unexpected shape can produce a confusing provider error far from the cause. The artifact signature check limits the risk to a trusted publisher, so this is hardening only.♻️ Proposed name-shape check
+const ENV_VAR_NAME_PATTERN = /^[A-Z_][A-Z0-9_]*$/i + function parseEnvVar(raw: unknown, index: number): ModuleConfigEnvVar {const name = requireNonEmptyString(obj.name, `${fieldPrefix}.name`) + if (!ENV_VAR_NAME_PATTERN.test(name)) { + fail( + `module.config.json field '${fieldPrefix}.name' must be a valid environment variable name, got: ${JSON.stringify(name)}`, + ) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/artifact/module-config.ts` around lines 106 - 118, Strengthen the env entry validation around the existing name parsing in the module configuration validator: after confirming name is non-empty, require it to match the deployment environment-variable naming rules, rejecting values containing “=”, newlines, or leading digits before returning the entry. Keep the existing requireNonEmptyString behavior and error context, and apply the check only to env[i].name.tests/cli/catalog.test.ts (2)
215-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
MAX_ARTIFACT_BYTESceiling.
fetchTarballrefuses a body overMAX_ARTIFACT_BYTES, andcli/src/catalog.tsdocuments that ceiling as a defence against a hostile signed-URL host. No test exercises it. A test also pins the ceiling as intentional behaviour if the read order changes later.💚 Proposed test
it('throws CatalogError on a non-ok response', async () => { const fetchImpl = fakeFetch(() => ({ ok: false, status: 403 })) await expect(fetchTarball('https://signed.example/x', fetchImpl)).rejects.toThrow(CatalogError) }) + + it('refuses a body larger than MAX_ARTIFACT_BYTES', async () => { + const oversized = new ArrayBuffer(MAX_ARTIFACT_BYTES + 1) + const fetchImpl = fakeFetch(() => ({ ok: true, status: 200, arrayBuffer: oversized })) + await expect(fetchTarball('https://signed.example/x', fetchImpl)).rejects.toThrow( + /exceeds the .* ceiling/, + ) + })Add
MAX_ARTIFACT_BYTESto the import list at line 2.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/catalog.test.ts` around lines 215 - 227, Add a fetchTarball test alongside the existing response and error tests that constructs a body exceeding MAX_ARTIFACT_BYTES and asserts the call rejects with CatalogError. Import MAX_ARTIFACT_BYTES from the catalog module so the test validates the documented ceiling without duplicating its value.
183-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the server message text is absent, not only the license key.
The test name states that the server-supplied message text never appears. The assertions only check that
sk_super_secret_valueis absent. The message'Missing or invalid license key.'does not contain the key, so a regression that appendedbody.error.messageto the thrown error would still pass here.Add an assertion against the message text.
💚 Proposed assertion
} catch (err) { expect(String(err)).not.toContain('sk_super_secret_value') + expect(String(err)).not.toContain('Missing or invalid license key') }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/catalog.test.ts` around lines 183 - 212, Add an assertion in the error-handling check for requestDownloadUrl that verifies the thrown error string does not contain the server-supplied message text “Missing or invalid license key.” Keep the existing license-key assertion and unauthorized-code assertion unchanged.cli/package.json (1)
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
filesfield before this package becomes publishable.
privateisfalse, sonpm publishwould succeed. The package has nofilesallowlist andbin/helpthread-module.jsreaches outside the package directory through../src/main.ts, which in turn imports the engine verifier at the repository root. A published tarball would not contain those files.The README states that a published CLI needs a bundling step, so this is deferred work. Consider setting
"private": trueuntil the bundling step exists, so an accidental publish cannot ship a broken package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/package.json` around lines 1 - 13, Prevent accidental publication of the incomplete helpthread-module package by changing the package.json private setting to true until the documented bundling step exists; do not add a files allowlist as a substitute while bin/helpthread-module.js still depends on ../src/main.ts and repository-root verifier code.tests/cli/verify-core.test.ts (1)
19-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an accept case for
verifyArtifact.Every test in this file asserts a rejection. Each one pins a distinct
code, so the failure modes are well covered. No test proves thatverifyArtifactreturnsok: truefor a genuine artifact. A regression that adds an over-strict check, or that rejects on a path none of these cases reach, passes this whole file.The real 88,107-byte artifact is not in the repository, so these vectors cannot drive a success path. Generate a keypair locally and pass it as the trust store, as
tests/cli/install.test.tsdoes at lines 20–34.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/verify-core.test.ts` around lines 19 - 87, Add a successful verifyArtifact test alongside the existing rejection cases, generating a local keypair and trust store as established in install.test.ts. Create or use a genuine artifact buffer, construct a matching manifest and valid signature with the generated private key, then assert verifyArtifact returns ok: true; keep the existing negative tests unchanged.cli/src/catalog.ts (1)
66-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the catalog body, and add a request timeout.
Two points on this function:
- Line 74 casts the parsed body to
CatalogResponsewith no runtime check. If the server returns a body withoutmodules,findModuleat line 79 throws a rawTypeErrorinstead of aCatalogError. This file treats a hostile marketplace as in scope, so check the shape here and throwCatalogError.- No call in this module sets a timeout.
FetchLikehas nosignalfield, so a server that never responds hangs the CLI. Add an optionalsignaltoFetchLikeand apply a deadline to each request.🔧 Proposed shape check
const res = await fetchImpl(`${catalogOrigin}/api/v1/modules`) if (!res.ok) { throw new CatalogError(`catalog request failed: HTTP ${res.status}`) } - return (await res.json()) as CatalogResponse + const body = (await res.json()) as CatalogResponse + if (!body || !Array.isArray(body.modules)) { + throw new CatalogError('catalog response is malformed: no `modules` array') + } + return body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/catalog.ts` around lines 66 - 75, Update fetchCatalog and FetchLike to enforce a request deadline using an AbortController signal, applying it to the catalog fetch and cleaning up the timeout afterward. Replace the unchecked CatalogResponse cast with runtime validation that confirms the parsed body has a valid modules collection; throw CatalogError for invalid shapes so downstream findModule calls never receive malformed data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/catalog.ts`:
- Around line 200-212: Update fetchTarball so the response body is consumed
incrementally and the download is aborted as soon as the running byte total
exceeds MAX_ARTIFACT_BYTES, before materializing the full payload; if FetchLike
only supports buffered bodies, first reject a Content-Length above the ceiling
and revise the fetchTarball documentation to describe the weaker guarantee
accurately.
In `@cli/src/install.ts`:
- Around line 121-148: Remove the unused temporary-file setup around
installVerifiedArtifact: delete the os/path-based temp directory creation,
tmpTarballPath, tarball write, and the try/finally cleanup block. Remove the
now-unused os import, and leave installVerifiedArtifact consuming tarballBytes
directly; also remove the inaccurate temp-directory comment.
- Around line 105-119: Update the version-handling logic around servedVersion
and servedEntry to reject the marketplace response when options.version is
explicitly set and differs from the served version. Throw an InstallError before
the entitlement NOTE and installation path; preserve the existing entitlement
behavior when no explicit version pin is provided.
In `@cli/src/prompt.ts`:
- Around line 37-57: Update the onData control-character comparisons to replace
the raw Ctrl-C and DEL literals with explicit '\u0003' and '\u007f' escape
sequences, while preserving the existing cancellation and backspace behavior.
- Around line 17-65: Update the prompt promise handling in both the TTY branch
and the readline-based non-TTY branch to settle when stdin emits error or
closes/ends, preventing hangs when input ends without a newline. Reject on stdin
errors and resolve or otherwise preserve the appropriate partial-input result on
normal closure, using the existing cleanup logic and ensuring handlers are
removed after settlement; anchor the changes around the prompt function, onData,
and rl.question.
In `@src/db/migrate.ts`:
- Around line 2172-2174: Update MIGRATION_031_WEBHOOK_ENDPOINTS_URL_UNIQUE to
remove duplicate webhook_endpoints rows before creating the unique index,
retaining one row per URL and deleting the remaining duplicates. Keep the
cleanup and index creation in the same migration so existing deployments with
duplicates can start successfully.
In `@src/modules/catalog/marketplace-client.test.ts`:
- Around line 281-291: Update the tampered response in the
downloadVerifiedArtifact test so its body has the same byte length as the
expected fixture while retaining different content, isolating the
sha256-mismatch assertion from size validation. Keep the existing size-mismatch
behavior covered separately if a dedicated case is present or needed.
In `@src/modules/catalog/marketplace-client.ts`:
- Around line 349-361: Update fetchFeed to validate every modules element before
casting to CatalogFeed, including the required versions collection used by
downloadVerifiedArtifact. Return the existing typed invalid-response failure for
malformed elements, ensuring malformed marketplace data never reaches
catalogModule.versions.find or causes an exception.
In `@src/modules/deploy/local-manual.ts`:
- Around line 174-180: Validate each key and value in setEnvVars before
constructing the .env.local lines, rejecting inputs containing newline
characters so hostile configuration cannot create additional declarations. Apply
the same validation to both key and value, and preserve the existing
file-writing behavior for valid entries.
In `@src/modules/deploy/vercel-adapter.ts`:
- Around line 553-555: Update URL normalization in createDeployment at
src/modules/deploy/vercel-adapter.ts#L553-L555 and getDeploymentState at
src/modules/deploy/vercel-adapter.ts#L578-L581 to recognize only http:// or
https:// schemes using the specified /^https?:\/\// test; otherwise prepend
https://.
In `@src/modules/install/installer.ts`:
- Around line 732-751: Update failInstall to classify or redact the supplied
reason before passing it into the transition detail and returning it as the
deadLetter reason. Ensure raw upstream errors from installation, artifact,
Vercel, deployment, and challenge-response callers never reach persistent
module_install_events.detail or queue dead-letter output, while preserving the
existing cleanup transition behavior.
In `@tests/cli/install.test.ts`:
- Around line 174-180: Update the runInstall test setup to explicitly stub
process.env.HELPTHREAD_LICENSE_KEY with a test license value before invoking it,
ensuring the environment lookup is deterministic. Revise the getLicenseKey
mock’s “unused-because-env-var-set” annotation to describe that the environment
variable is intentionally set and takes precedence.
In `@tests/modules/artifact/tar-helpers.ts`:
- Around line 55-59: Update the checksum-field construction in the tar header
generation logic around writeOctalField so the checksum writes exactly six octal
digits into bytes 148–153, preserving the least significant digit; set byte 154
to NUL and byte 155 to space, matching the USTAR field layout. Adjust the write
parameters or helper usage without changing checksum calculation.
---
Nitpick comments:
In `@cli/package.json`:
- Around line 1-13: Prevent accidental publication of the incomplete
helpthread-module package by changing the package.json private setting to true
until the documented bundling step exists; do not add a files allowlist as a
substitute while bin/helpthread-module.js still depends on ../src/main.ts and
repository-root verifier code.
In `@cli/src/catalog.ts`:
- Around line 66-75: Update fetchCatalog and FetchLike to enforce a request
deadline using an AbortController signal, applying it to the catalog fetch and
cleaning up the timeout afterward. Replace the unchecked CatalogResponse cast
with runtime validation that confirms the parsed body has a valid modules
collection; throw CatalogError for invalid shapes so downstream findModule calls
never receive malformed data.
In `@cli/src/install.ts`:
- Around line 32-33: Update the getLicenseKey doc comment in the install
interface to describe only its license-key retrieval behavior, removing the
claim that it checks HELPTHREAD_LICENSE_KEY first. Keep the environment-variable
check documented at the existing line 78 location where that responsibility is
implemented.
In `@cli/src/main.ts`:
- Around line 93-101: Update the rejection handler for main() to print a concise
user-facing message for unexpected errors before any stack trace. Preserve clean
messages for InstallError and VerifyCommandError, and only include the stack
when the HELPTHREAD_DEBUG environment flag is enabled; otherwise avoid exposing
raw TypeError stack output from fetchCatalog or fetchTarball.
In `@src/modules/artifact/extract.ts`:
- Around line 296-300: Update the validation loop in extract.ts to reject any
archive entry whose path is a descendant of an earlier regular-file entry,
including conflicts such as “a” followed by “a/b”; preserve the existing
UnsafeArchiveError refusal behavior. Also wrap the write phase around the loop
over toWrite, including mkdirSync, writeFileSync, and written updates, so
unexpected filesystem failures are caught and rethrown as UnsafeArchiveError
rather than leaking raw errors or leaving partial writes unreported.
In `@src/modules/artifact/module-config.ts`:
- Around line 106-118: Strengthen the env entry validation around the existing
name parsing in the module configuration validator: after confirming name is
non-empty, require it to match the deployment environment-variable naming rules,
rejecting values containing “=”, newlines, or leading digits before returning
the entry. Keep the existing requireNonEmptyString behavior and error context,
and apply the check only to env[i].name.
In `@src/modules/catalog/marketplace-client.test.ts`:
- Around line 156-198: Expand the downloadVerifiedArtifact test suite with
refusal cases for bad-signature and untrusted-key, using setup, makeTestKeypair,
and trustStore: sign the manifest with a second keypair while retaining the
catalog’s trusted keyId for bad-signature, and remove the keyId from trustStore
for untrusted-key. Also add coverage for served-version-mismatch and
manifest-parse-error, asserting each result reports its corresponding refusal
code.
In `@src/modules/deploy/vercel-adapter.test.ts`:
- Around line 151-171: Rename the `describe('createVercelDeployProvider —
allowlist')` block and its test to reflect that they cover `assertSafeId`
rejection of an empty `deploymentId`, not allowlist enforcement. Do not label
this case as allowlist coverage; add a separate direct `assertAllowlisted` case
only if allowlist behavior must also be tested.
In `@src/modules/deploy/vercel-adapter.ts`:
- Around line 302-321: Update readBoundedText’s cap-exceeded path to cancel the
body reader before propagating VercelAdapterError. Ensure reader.cancel() runs
when total exceeds capBytes, while retaining the existing releaseLock cleanup
and normal streaming behavior.
In `@src/store/module-license.test.ts`:
- Around line 14-35: Update freshDb to call the existing migrate(db) helper
instead of maintaining inline CREATE TABLE DDL, once the module_license
migration is available. Remove the duplicated schema definition while preserving
creation of a fresh PGlite database and returning the initialized Db instance.
In `@src/store/module-license.ts`:
- Around line 233-244: Update ModuleLicenseRow.updated_at and
LicenseDisplayInfo.updatedAt to accept Date | string, then normalize
row.updated_at through the existing toDate utility in getDisplayInfo before
constructing the returned display object.
In `@tests/cli/catalog.test.ts`:
- Around line 215-227: Add a fetchTarball test alongside the existing response
and error tests that constructs a body exceeding MAX_ARTIFACT_BYTES and asserts
the call rejects with CatalogError. Import MAX_ARTIFACT_BYTES from the catalog
module so the test validates the documented ceiling without duplicating its
value.
- Around line 183-212: Add an assertion in the error-handling check for
requestDownloadUrl that verifies the thrown error string does not contain the
server-supplied message text “Missing or invalid license key.” Keep the existing
license-key assertion and unauthorized-code assertion unchanged.
In `@tests/cli/env-summary.test.ts`:
- Around line 62-72: Strengthen the test around renderEnvSummary by selecting
the operator environment fixture from CONFIG.env by its identifying name rather
than index, then add coverage for the engine-managed empty side as well as the
existing operator-managed empty side. Preserve the "(none)" assertions for both
summary sections.
In `@tests/cli/verify-core.test.ts`:
- Around line 19-87: Add a successful verifyArtifact test alongside the existing
rejection cases, generating a local keypair and trust store as established in
install.test.ts. Create or use a genuine artifact buffer, construct a matching
manifest and valid signature with the generated private key, then assert
verifyArtifact returns ok: true; keep the existing negative tests unchanged.
In `@tests/modules/artifact/extract.test.ts`:
- Around line 26-39: Add a test in the “safeExtract: destination preconditions”
suite that creates a symlink as the destination path and verifies safeExtract
rejects it, asserting the expected refusal error. Use the existing destDir and
temporary path setup without altering the current missing and non-empty
destination tests.
- Around line 130-159: Reduce memory usage in the artifact cap tests around the
oversized entries and archive-cap case by using sparse/minimal file bodies while
preserving their declared sizes through the existing tar-entry size override
mechanism, such as sizeOverride/paxSizeOverride. Ensure buildTarGz still
produces headers declaring MAX_SINGLE_FILE_BYTES or the over-cap total so
safeExtract rejects for the same size-limit reasons without allocating 128–512
MiB buffers.
In `@tests/modules/artifact/module-config.test.ts`:
- Around line 8-9: Update the comment above REAL_DRAFT_ASSISTANT_CONFIG to
remove the “byte-for-byte” claim and state that the fixture mirrors the shipped
configuration’s contents or values, without implying preservation of whitespace
or key order.
In `@tests/modules/artifact/vectors.test.ts`:
- Around line 78-81: Update the unknown-key fixture in the test around
parseManifest to avoid string replacement: parse MANIFEST_JSON, add an
unexpected field to the resulting manifest object, and serialize it back to JSON
before calling parseManifest. Preserve the existing assertion that the parser
rejects the added unknown key.
In `@tsconfig.json`:
- Line 14: Update the TypeScript project configuration around the root include
entry so cli/src is no longer merged into the engine’s root program. Either add
an explicit lint rule preventing src modules such as marketplace-client from
importing cli code, or give cli its own tsconfig.json and reference it as a
separate project while preserving independent compilation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bb16c5bb-09ce-40c0-bd17-505edd029b5d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (47)
cli/README.mdcli/bin/helpthread-module.jscli/package.jsoncli/src/args.tscli/src/catalog.tscli/src/env-summary.tscli/src/install.tscli/src/main.tscli/src/prompt.tscli/src/trust-store.tscli/src/verify-core.tscli/src/verify.tspackage.jsonsrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/modules/artifact/extract.tssrc/modules/artifact/index.tssrc/modules/artifact/manifest.tssrc/modules/artifact/module-config.tssrc/modules/catalog/marketplace-client.test.tssrc/modules/catalog/marketplace-client.tssrc/modules/deploy/local-manual.tssrc/modules/deploy/provider.tssrc/modules/deploy/vercel-adapter.test.tssrc/modules/deploy/vercel-adapter.tssrc/modules/install/challenge.test.tssrc/modules/install/challenge.tssrc/modules/install/installer.test.tssrc/modules/install/installer.tssrc/store/module-installs.test.tssrc/store/module-installs.tssrc/store/module-license.test.tssrc/store/module-license.tssrc/store/vercel-connection.test.tssrc/store/vercel-connection.tssrc/store/webhook-endpoints.tstests/cli/args.test.tstests/cli/catalog.test.tstests/cli/env-summary.test.tstests/cli/install.test.tstests/cli/verify-core.test.tstests/modules/artifact/extract.test.tstests/modules/artifact/module-config.test.tstests/modules/artifact/tar-helpers.tstests/modules/artifact/vectors.test.tstsconfig.json
Comment on lines
+200
to
+212
| export async function fetchTarball(url: string, fetchImpl: FetchLike): Promise<Buffer> { | ||
| const res = await fetchImpl(url) | ||
| if (!res.ok) { | ||
| throw new CatalogError(`tarball download failed: HTTP ${res.status}`) | ||
| } | ||
| const arrayBuffer = await res.arrayBuffer() | ||
| if (arrayBuffer.byteLength > MAX_ARTIFACT_BYTES) { | ||
| throw new CatalogError( | ||
| `refusing the download: ${arrayBuffer.byteLength} bytes exceeds the ${MAX_ARTIFACT_BYTES}-byte ceiling for a module artifact.`, | ||
| ) | ||
| } | ||
| return Buffer.from(arrayBuffer) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The size ceiling runs after the whole body is already in memory.
res.arrayBuffer() materializes the complete response body. The length check then runs against a buffer that already exists. The doc comment at lines 188–196 states that this ceiling stops a hostile signed-URL host from exhausting memory "long before any digest or signature check runs". The current order does not achieve that.
Stream the body and abort once the running total passes MAX_ARTIFACT_BYTES. If FetchLike stays body-buffered, then reject an advertised Content-Length above the ceiling first, and correct the comment so it does not overstate the guarantee.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/catalog.ts` around lines 200 - 212, Update fetchTarball so the
response body is consumed incrementally and the download is aborted as soon as
the running byte total exceeds MAX_ARTIFACT_BYTES, before materializing the full
payload; if FetchLike only supports buffered bodies, first reject a
Content-Length above the ceiling and revise the fetchTarball documentation to
describe the weaker guarantee accurately.
Comment on lines
+105
to
+119
| const servedVersion = download.version ?? versionEntry.version | ||
| const servedEntry = | ||
| servedVersion === versionEntry.version | ||
| ? versionEntry | ||
| : mod.versions.find((v) => v.version === servedVersion) | ||
| if (servedEntry === undefined) { | ||
| throw new InstallError( | ||
| `the marketplace served version ${servedVersion}, which is not in its own public feed — refusing to install an artifact whose manifest cannot be independently located.`, | ||
| ) | ||
| } | ||
| if (servedEntry !== versionEntry) { | ||
| deps.log( | ||
| ` NOTE: your license entitles you to ${servedEntry.version}, not ${versionEntry.version}. Installing ${servedEntry.version}.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== install.ts outline =="
ast-grep outline cli/src/install.ts --view expanded || true
echo "== install.ts relevant lines =="
cat -n cli/src/install.ts | sed -n '1,240p'
echo "== catalog download/response/retrieve relevant lines =="
cat -n cli/src/catalog.ts | sed -n '1,240p'
echo "== args types =="
cat -n cli/src/args.ts | sed -n '1,80p'Repository: Helpthread/helpthread
Length of output: 25097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests matching version/served/version substitution =="
rg -n "served|version|entitl|entitlement|--version|download\.version|DownloadResponse|ver.*pinned|pin" tests/cli/install.test.ts cli/src/install.ts cli/src/args.ts -C 3 || true
echo "== install test relevant sections =="
wc -l tests/cli/install.test.ts
sed -n '1,320p' tests/cli/install.test.ts
sed -n '420,680p' tests/cli/install.test.tsRepository: Helpthread/helpthread
Length of output: 37971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const resolve = (requestedVersion, versions) => {
if (requestedVersion) {
const found = versions.find((v) => v.version === requestedVersion)
if (!found) throw new Error(`module has no published version '${requestedVersion}'`)
return found
}
return versions.filter((v) => v.yanked !== true).reduce((a, b) =>
a.version.split('.').map(Number).join('') >= b.version.split('.').map(Number).join('') ? a : b
)
}
const versions = [
{ version: '2.0.0' },
{ version: '1.0.0' },
]
for (const optionsVersion of [undefined, '2.0.0']) {
const versionEntry = resolve(optionsVersion, versions)
const download = { version: '1.0.0' }
const servedVersion = download.version ?? versionEntry.version
const servedEntry =
servedVersion === versionEntry.version
? versionEntry
: versions.find((v) => v.version === servedVersion)
console.log(JSON.stringify({
optionsVersion,
versionEntry: versionEntry.version,
servedEntry: servedEntry.version,
installVersionMatchesOptionsPin: optionsVersion === undefined || servedEntry.version === optionsVersion,
installedVersionChanged: versionEntry.version !== servedEntry.version,
}))
}
JSRepository: Helpthread/helpthread
Length of output: 419
Security And Privacy (CWE-494): Download of Code Without Integrity Check
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
cli/src/main.ts:20
main
│
▼
● Sink
cli/src/install.ts
Enforce an explicit --version pin against the served version.
download.version comes from the marketplace response. When servedEntry differs and options.version is set, this code continues to install servedEntry.version and only prints the entitlement NOTE. Refuse when the marketplace serves a different version than the operator pin.
🛡️ Proposed fix
if (servedEntry !== versionEntry) {
+ if (options.version !== undefined) {
+ throw new InstallError(
+ `refusing to install: you pinned version ${options.version} but the marketplace served ${servedEntry.version}. Re-run without --version to install the version your license entitles you to.`,
+ )
+ }
deps.log(
` NOTE: your license entitles you to ${servedEntry.version}, not ${versionEntry.version}. Installing ${servedEntry.version}.`,
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const servedVersion = download.version ?? versionEntry.version | |
| const servedEntry = | |
| servedVersion === versionEntry.version | |
| ? versionEntry | |
| : mod.versions.find((v) => v.version === servedVersion) | |
| if (servedEntry === undefined) { | |
| throw new InstallError( | |
| `the marketplace served version ${servedVersion}, which is not in its own public feed — refusing to install an artifact whose manifest cannot be independently located.`, | |
| ) | |
| } | |
| if (servedEntry !== versionEntry) { | |
| deps.log( | |
| ` NOTE: your license entitles you to ${servedEntry.version}, not ${versionEntry.version}. Installing ${servedEntry.version}.`, | |
| ) | |
| } | |
| const servedVersion = download.version ?? versionEntry.version | |
| const servedEntry = | |
| servedVersion === versionEntry.version | |
| ? versionEntry | |
| : mod.versions.find((v) => v.version === servedVersion) | |
| if (servedEntry === undefined) { | |
| throw new InstallError( | |
| `the marketplace served version ${servedVersion}, which is not in its own public feed — refusing to install an artifact whose manifest cannot be independently located.`, | |
| ) | |
| } | |
| if (servedEntry !== versionEntry) { | |
| if (options.version !== undefined) { | |
| throw new InstallError( | |
| `refusing to install: you pinned version ${options.version} but the marketplace served ${servedEntry.version}. Re-run without --version to install the version your license entitles you to.`, | |
| ) | |
| } | |
| deps.log( | |
| ` NOTE: your license entitles you to ${servedEntry.version}, not ${versionEntry.version}. Installing ${servedEntry.version}.`, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/install.ts` around lines 105 - 119, Update the version-handling logic
around servedVersion and servedEntry to reject the marketplace response when
options.version is explicitly set and differs from the served version. Throw an
InstallError before the entitlement NOTE and installation path; preserve the
existing entitlement behavior when no explicit version pin is provided.
Comment on lines
+121
to
+148
| deps.log('4. Downloading the release artifact...') | ||
| const tarballBytes = await fetchTarball(download.downloadUrl, deps.fetchImpl) | ||
| const tmpDir = fsImpl.mkdtempSync(path.join(os.tmpdir(), 'helpthread-module-')) | ||
| const tmpTarballPath = path.join(tmpDir, `${mod.slug}-${servedEntry.version}.tar.gz`) | ||
| fsImpl.writeFileSync(tmpTarballPath, tarballBytes) | ||
|
|
||
| // Everything from here runs inside try/finally: this is PAID, proprietary | ||
| // software sitting in a world-readable temp directory, and any failure | ||
| // below — a refused destination, a bad archive entry, a missing config — | ||
| // would otherwise leave it there indefinitely. Cleanup must not depend on | ||
| // reaching a particular branch. | ||
| try { | ||
| await installVerifiedArtifact({ | ||
| deps, | ||
| fsImpl, | ||
| trustStore, | ||
| options, | ||
| mod, | ||
| servedEntry, | ||
| tarballBytes, | ||
| }) | ||
| } finally { | ||
| try { | ||
| fsImpl.rmSync(tmpDir, { recursive: true, force: true }) | ||
| } catch { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused temp-file write.
tmpTarballPath is written at Line 125 and never read. installVerifiedArtifact verifies and extracts from the in-memory tarballBytes, so the temp copy has no consumer. The write places paid proprietary bytes on disk for no benefit, and it depends on a best-effort rmSync that can fail silently. Delete the temp directory, the write, and the try/finally that exists only to clean them up.
Note also that the comment at Line 128 describes the temp directory as world-readable. fs.mkdtempSync creates the directory with mode 0700, so that description is inaccurate.
♻️ Proposed fix
deps.log('4. Downloading the release artifact...')
const tarballBytes = await fetchTarball(download.downloadUrl, deps.fetchImpl)
- const tmpDir = fsImpl.mkdtempSync(path.join(os.tmpdir(), 'helpthread-module-'))
- const tmpTarballPath = path.join(tmpDir, `${mod.slug}-${servedEntry.version}.tar.gz`)
- fsImpl.writeFileSync(tmpTarballPath, tarballBytes)
-
- // Everything from here runs inside try/finally: this is PAID, proprietary
- // software sitting in a world-readable temp directory, and any failure
- // below — a refused destination, a bad archive entry, a missing config —
- // would otherwise leave it there indefinitely. Cleanup must not depend on
- // reaching a particular branch.
- try {
- await installVerifiedArtifact({
- deps,
- fsImpl,
- trustStore,
- options,
- mod,
- servedEntry,
- tarballBytes,
- })
- } finally {
- try {
- fsImpl.rmSync(tmpDir, { recursive: true, force: true })
- } catch {
- // best-effort cleanup
- }
- }
+
+ await installVerifiedArtifact({
+ deps,
+ fsImpl,
+ trustStore,
+ options,
+ mod,
+ servedEntry,
+ tarballBytes,
+ })
}Remove the now-unused os import as well:
import * as fs from 'node:fs'
-import * as os from 'node:os'
import * as path from 'node:path'If a later change reintroduces a file-based verification path, keep the temp directory and pass tmpTarballPath into installVerifiedArtifact so the write has a consumer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deps.log('4. Downloading the release artifact...') | |
| const tarballBytes = await fetchTarball(download.downloadUrl, deps.fetchImpl) | |
| const tmpDir = fsImpl.mkdtempSync(path.join(os.tmpdir(), 'helpthread-module-')) | |
| const tmpTarballPath = path.join(tmpDir, `${mod.slug}-${servedEntry.version}.tar.gz`) | |
| fsImpl.writeFileSync(tmpTarballPath, tarballBytes) | |
| // Everything from here runs inside try/finally: this is PAID, proprietary | |
| // software sitting in a world-readable temp directory, and any failure | |
| // below — a refused destination, a bad archive entry, a missing config — | |
| // would otherwise leave it there indefinitely. Cleanup must not depend on | |
| // reaching a particular branch. | |
| try { | |
| await installVerifiedArtifact({ | |
| deps, | |
| fsImpl, | |
| trustStore, | |
| options, | |
| mod, | |
| servedEntry, | |
| tarballBytes, | |
| }) | |
| } finally { | |
| try { | |
| fsImpl.rmSync(tmpDir, { recursive: true, force: true }) | |
| } catch { | |
| // best-effort cleanup | |
| } | |
| } | |
| deps.log('4. Downloading the release artifact...') | |
| const tarballBytes = await fetchTarball(download.downloadUrl, deps.fetchImpl) | |
| await installVerifiedArtifact({ | |
| deps, | |
| fsImpl, | |
| trustStore, | |
| options, | |
| mod, | |
| servedEntry, | |
| tarballBytes, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/install.ts` around lines 121 - 148, Remove the unused temporary-file
setup around installVerifiedArtifact: delete the os/path-based temp directory
creation, tmpTarballPath, tarball write, and the try/finally cleanup block.
Remove the now-unused os import, and leave installVerifiedArtifact consuming
tarballBytes directly; also remove the inaccurate temp-directory comment.
Comment on lines
+17
to
+65
| return new Promise((resolve, reject) => { | ||
| const stdin = process.stdin | ||
| const stdout = process.stdout | ||
| stdout.write(promptText) | ||
|
|
||
| if (!stdin.isTTY) { | ||
| const rl = readline.createInterface({ input: stdin, output: undefined, terminal: false }) | ||
| rl.question('', (answer) => { | ||
| rl.close() | ||
| resolve(answer) | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| const wasRaw = stdin.isRaw | ||
| stdin.setRawMode(true) | ||
| stdin.resume() | ||
| stdin.setEncoding('utf8') | ||
|
|
||
| let value = '' | ||
| const onData = (chunk: string) => { | ||
| for (const ch of chunk) { | ||
| if (ch === '\n' || ch === '\r') { | ||
| cleanup() | ||
| stdout.write('\n') | ||
| resolve(value) | ||
| return | ||
| } | ||
| if (ch === '') { | ||
| // Ctrl-C | ||
| cleanup() | ||
| reject(new Error('prompt cancelled')) | ||
| return | ||
| } | ||
| if (ch === '' || ch === '\b') { | ||
| // Backspace | ||
| value = value.slice(0, -1) | ||
| continue | ||
| } | ||
| value += ch | ||
| } | ||
| } | ||
| const cleanup = () => { | ||
| stdin.removeListener('data', onData) | ||
| stdin.setRawMode(Boolean(wasRaw)) | ||
| stdin.pause() | ||
| } | ||
| stdin.on('data', onData) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Settle the promise when stdin closes or errors.
Neither branch handles stdin ending or erroring. In the TTY branch, onData resolves only on a newline. In the non-TTY branch, rl.question invokes its callback only when a line arrives. If stdin closes first, for example a piped input with no trailing newline or a closed pipe, the promise never settles and the CLI hangs with no output.
Add error and close/end handlers that reject or resolve.
🛡️ Proposed fix
if (!stdin.isTTY) {
const rl = readline.createInterface({ input: stdin, output: undefined, terminal: false })
+ rl.once('close', () => reject(new Error('stdin closed before a license key was provided')))
rl.question('', (answer) => {
rl.close()
resolve(answer)
})
return
} stdin.on('data', onData)
+ stdin.once('error', (err) => {
+ cleanup()
+ reject(err)
+ })
+ stdin.once('end', () => {
+ cleanup()
+ reject(new Error('stdin closed before a license key was provided'))
+ })
})
}Promise ignores a settle call after the first one, so the close handler is safe to register before rl.question resolves.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/prompt.ts` around lines 17 - 65, Update the prompt promise handling
in both the TTY branch and the readline-based non-TTY branch to settle when
stdin emits error or closes/ends, preventing hangs when input ends without a
newline. Reject on stdin errors and resolve or otherwise preserve the
appropriate partial-input result on normal closure, using the existing cleanup
logic and ensuring handlers are removed after settlement; anchor the changes
around the prompt function, onData, and rl.question.
Comment on lines
+37
to
+57
| const onData = (chunk: string) => { | ||
| for (const ch of chunk) { | ||
| if (ch === '\n' || ch === '\r') { | ||
| cleanup() | ||
| stdout.write('\n') | ||
| resolve(value) | ||
| return | ||
| } | ||
| if (ch === '') { | ||
| // Ctrl-C | ||
| cleanup() | ||
| reject(new Error('prompt cancelled')) | ||
| return | ||
| } | ||
| if (ch === '' || ch === '\b') { | ||
| // Backspace | ||
| value = value.slice(0, -1) | ||
| continue | ||
| } | ||
| value += ch | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Reveal any raw control bytes in the prompt's character comparisons.
sed -n '36,58p' cli/src/prompt.ts | cat -ARepository: Helpthread/helpthread
Length of output: 723
Replace raw control literals with explicit escape sequences.
cat -A shows raw Ctrl-C (^C) and DEL (^?) characters in these comparisons. Any editor or copy-paste step can strip or alter these invisible bytes, so handle them with named constants using '\u0003' for Ctrl-C and '\u007f' for DEL.
🐛 Proposed fix
+const CTRL_C = '\u0003'
+const DEL = '\u007f'
+
- if (ch === '^C') {
- // Ctrl-C
+ if (ch === CTRL_C) {
cleanup()
reject(new Error('prompt cancelled'))
return
}
- if (ch === '^?' || ch === '\b') {
- // Backspace
+ if (ch === DEL || ch === '\b') {
value = value.slice(0, -1)
continue
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/prompt.ts` around lines 37 - 57, Update the onData control-character
comparisons to replace the raw Ctrl-C and DEL literals with explicit '\u0003'
and '\u007f' escape sequences, while preserving the existing cancellation and
backspace behavior.
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
The operator's own engine installs paid modules into the operator's own Vercel account. Resonant IQ holds no operator credential and hosts nothing — charter-level, and the reason every piece here is shaped the way it is. Adds migration 030/031 (vercel_connections, module_installs, module_install_events, and a unique index on webhook_endpoints.url), a fenced state machine with real leases, a vendor-neutral DeployProvider with Vercel and local-manual implementations, encrypted credential stores under their own key class, a server-side catalog client reusing src/modules/artifact, and the queued installer composing them. Three adversarial review rounds. Round one found the root defect: there was no lease. The fence token sat readable on the row, so N workers all held a valid fence and all executed side effects — only the final write serialized. Downstream of it: duplicate Vercel projects, duplicate LIVE webhook endpoints delivering every event twice forever, installs reaching 'active' holding an Assistant token the database no longer recognized, and a revoked hosting connection tearing down a RUNNING module. Round two verified those closed and found six defects the fixes introduced, including a reproduced CRITICAL: a crash between the endpoint CAS and its promote left the lease held, and the retry's refused claim returned ack — telling the queue the work was done and deleting the job. An ack asserts the work is finished; a held lease only asserts someone else holds it, and that someone may be dead. Round three closed those, each pinned by a test watched failing against the reverted code. Stated rather than hidden: this ships no HTTP surface, so the step-up condition is deferred by construction; the Vercel REST paths are written from memory and marked unverified against a live account; and the dedicated encryption key is contract-only until a composition root wires it. 1958 tests, typecheck clean, Biome 362 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four findings, all real, all pinned by tests watched failing against reverted code. Artifact config is now allowlisted, not denylisted. Rejecting five named keys left every other directive Vercel honors — legacy builds, functions, routes, regions — free to ride along in a hostile artifact. A denylist has to enumerate a vendor's entire evolving surface and loses to the next key they ship. Any vercel.json is refused outright (a prebuilt artifact needs none) and config.json is restricted to the keys prebuilt output genuinely requires, failing closed on anything unknown or malformed. Credentials leave the permanent audit table. The crash-resume path escrowed the Assistant token and webhook secret as ciphertext inside module_install_events.detail — a table that is append-only and permanent by design, so decryptable credentials would have persisted forever. The recovery need was real; the location was wrong. Migration 032 adds a dedicated escrow table, deleted on activation or any terminal state; the audit event keeps only its opaque id. Cleanup is retryable instead of best-effort. Revocation and endpoint disabling ran after the row was already terminal with errors discarded, so a transient database failure could leave an Assistant token live, or an endpoint still receiving real customer events, with nothing to retry it. Migration 033 adds cleanup_pending: failure fences there first, and the terminal state commits only once revocation and disabling both actually succeed. The lease is renewed across long operations. Five minutes was never extended through catalog download, extraction, sequential uploads, or build polling, so a slow install could have its row reclaimed while it was still creating real Vercel resources — the duplicate-resource class the lease exists to prevent. Renewal is fenced, and losing the fence aborts before the next provider call rather than completing work another worker now owns. 1974 tests, typecheck clean, Biome 362 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A cleanup that threw returned retry while still holding its lease: the handler's outer finally releases with the token it read at entry, which the cleanup_pending transition had already rotated away. Every redelivery in that window lost its claim and returned retry, spending the queue's attempt budget on waiting rather than on cleanup — and with a low maxAttempts the job dead-letters, stranding the install at cleanup_pending holding a live Assistant token. That is the exact state the retryable-cleanup path exists to prevent. The regression test previously rewound lease_expires_at by hand, which hid this. It now asserts the row is genuinely claimable with no help, then hands the lease straight back so the redelivery it is testing can take it. 1974 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All real, all fixed.
Migration 031 added a unique index on webhook_endpoints.url with no
de-duplication step. Our own production holds zero rows, so we were
never at risk — but this is the public repo other operators self-host,
migrations here are forward-only and applied by hand, and any deployment
carrying a duplicate URL would have hit an aborted migration that blocked
every later one too. It now de-duplicates deterministically first,
keeping the newest row per URL and disabling-and-renaming the losers
rather than deleting anything.
Install failures interpolated provider and network error text straight
into module_install_events.detail — permanent storage receiving strings
of unknown provenance. Failures are now a closed set of reason codes.
That is the third finding in this class on this file; the audit table
must only ever receive values chosen deliberately.
downloadVerifiedArtifact collapsed distinct failures into one hardcoded
code, discarding the precise reason its callee had already determined,
and a malformed feed element threw where the contract promises a typed
failure. Both now propagate honestly.
url.startsWith('http') tested a prefix rather than a scheme, so a project
named http-anything produced a URL with no scheme. Env writing rejects
newlines in keys and values instead of letting one inject extra lines.
A test helper wrote tar checksums a digit short, and a tamper test
changed length as well as content, so it never isolated the digest check.
1983 tests, typecheck clean, Biome 363 files.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zaridan
force-pushed
the
feat/ht-119-engine-deployer
branch
from
August 4, 2026 16:19
4dce537 to
9c55f62
Compare
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 32 minutes. |
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
2 participants
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.
🟢 SAFE TO MERGE
Gates green on the rebased head (Quality: typecheck/lint/test/coverage; gitleaks; CodeQL adjudicated below). Maintainer instructed merge, 2026-08-04: "merge both PRs" / "merge them once the fixes land".
CodeRabbit: 7 findings — all real, all fixed (commit on this branch). Re-review requested after the fix push AND again after the rebase; both returned zero new inline comments.
CodeQL: fail adjudicated as not-ours. Zero alerts in
src/on this branch. The high-severity items are OpenSSF Scorecard meta-checks already failing onmain(approved-changesets count, repository age, an unpinned workflow action). None introduced here.Rebased onto main after #186 squash-merged, replaying only this branch's four commits. Verified locally after rebase: 1999 tests / 105 files, typecheck clean, Biome 363 files — the HT-119 work composes with the CLI review fixes that landed in #186.
The finding worth noting: migration 031 added a unique index on
webhook_endpoints.urlwith no de-duplication. Our own production holds zero rows (verified by query), so we were never at risk — but this is the public repo other operators self-host, migrations are forward-only and applied by hand, and any deployment carrying a duplicate would have hit an aborted migration blocking every later one. It now de-duplicates deterministically first.The three gaps below are unchanged and still true after merge — this is a foundation, not a shippable feature.
Original verdict at open — 🟡 NEEDS YOUR DECISION
Foundation only — this ships no callable surface. Green (1974 tests, typecheck, Biome 362 files) and reviewed four times, but three gaps are stated below rather than hidden, and one architectural decision is mine and needs your confirmation.
Reviews: two Opus passes → verification pass → Codex different-vendor pass (in place of CodeRabbit, not installed on this org) → final verification. 20 findings across four rounds, all closed. Trajectory 10 → 6 → 4 → 1, severity falling from "there is no lease" to "the lease isn't released on one failure branch."
Decision provenance
What this is
The operator's own engine installs paid modules into the operator's own Vercel account. Resonant IQ holds no operator credential and hosts nothing — that constraint shaped every piece.
Migrations 030–033; a
module_installsstate machine with real claim-based leases, CAS transitions and an append-only audit;vercel_connections(encrypted, team-bound, immutable team id); a vendor-neutralDeployProviderwith Vercel and local-manual implementations; encrypted credential stores under their own key class; a server-side catalog client reusing the samesrc/modules/artifactlibrary the CLI uses; and the queued installer composing them.What this does NOT do — please read before treating HT-119 as closed
HELPTHREAD_VERCEL_TOKEN_ENC_KEY. The store takes the key as a parameter so nothing can silently reuse the mailbox key — but nothing yet proves it at runtime.{version, routes}). Our own artifact only needsversion, so nothing breaks today, but the first module needingoverridesgets refused. Deliberately erring tight; needs widening with justification per key.The findings worth your attention
Round 1 found there was no lease at all. The "fence token" sat readable on the row, so N workers all held a valid fence and all executed side effects — only the final write serialized. Downstream: duplicate Vercel projects, duplicate live webhook endpoints delivering every conversation event twice forever, and installs reaching
activeholding an Assistant token the database no longer recognized. Also: a revoked hosting connection tore down a running module, violating the charter-level rule that credential failures never disturb running software.Round 2 found the fixes had introduced a new CRITICAL, and reproduced it. A crash between the endpoint transition and its promote left the lease held; the retry's refused claim returned
ack— telling the queue the work was done and deleting the job. Anackasserts the work is finished; a held lease only asserts someone else holds it, and that someone may be dead.Codex found that an earlier fix had created a new problem. Escrowing credential ciphertext in the transition's
detailput it inmodule_install_events— permanent and append-only by design, so decryptable credentials would have persisted forever. The recovery need was real; the location was wrong. Migration 032 moves it to a dedicated escrow table deleted on activation or terminal state.Disclosure: a pre-existing test named "does not treat an unparseable vercel.json as evidence of anything" was rewritten to expect rejection. It was asserting the exact hole being closed — a test can encode a bug as easily as a behavior — but changing a test to make a fix pass deserves to be visible, not buried.
Verified, not asserted
1974 tests / 104 files; typecheck clean; Biome 362 files. Every round-3 and round-4 fix was pinned by a regression test watched failing against the reverted code. I independently traced that no key material reaches the audit table, and confirmed RLS on all new tables.
🤖 Generated with Claude Code
Summary by CodeRabbit
helpthread-moduleCLI with module installation and offline artifact verification commands.