feat(modules): artifact verification library + helpthread-module CLI (HT-116) - #186

tests/modules/artifact/module-config.test.ts (1)

8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Soften the "byte-for-byte" claim.

The fixture is rebuilt with JSON.stringify from an object literal. The test therefore asserts the shape and values, not the exact bytes of the shipped file. tests/modules/artifact/vectors.test.ts uses a literal string where byte fidelity matters. Either state that this fixture mirrors the shipped values, or inline the literal JSON text.

♻️ Proposed wording

-/** The REAL module.config.json shipped in the draft-assistant 0.3.0 release artifact, byte-for-byte. */
+/** The values of the module.config.json shipped in the draft-assistant 0.3.0 release artifact. Re-serialized here, so this fixture is value-identical, not byte-identical. */

🤖 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
REAL_DRAFT_ASSISTANT_CONFIG comment to describe that the fixture mirrors the
shipped configuration’s values and structure, rather than claiming byte-for-byte
identity; keep the existing JSON.stringify-based fixture unchanged.

tests/modules/artifact/vectors.test.ts (2)

58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align the comment with the mutation the test performs.

The comment states that the final character changes from 9 to 8. The code removes "} and appends 8"}, so sourceRevision becomes 23230a98. The assertion still holds, but the comment describes different bytes than the test uses.

♻️ Proposed comment or code alignment

-    // Flip the final character of sourceRevision from '9' to '8'.
+    // Replace the final character of sourceRevision, '9', with '8'.
     expect(MANIFEST_JSON.endsWith('"sourceRevision":"23230a9"}')).toBe(true)
-    const flipped = `${MANIFEST_JSON.slice(0, -2)}8"}`
+    const flipped = `${MANIFEST_JSON.slice(0, -3)}8"}`

🤖 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 58 - 64, Update the
comment in the test case around verifyManifestSignature to accurately describe
the mutation performed: the sourceRevision changes from 23230a9 to 23230a98 by
appending 8 before the closing quote and brace. Keep the existing mutation and
assertions unchanged.

---

83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a vector for the non-canonical base64url branch.

decodeBase64UrlStrict has three rejection paths: alphabet, decoded length, and the re-encode round trip. The tests cover the alphabet path at Line 69 and the success path here. The round-trip path stays untested. A signature whose final character sets non-zero padding bits decodes to 64 bytes and passes the first two checks, so this branch is the one most likely to regress silently.

♻️ Proposed additional vector

   it('decodeBase64UrlStrict independently confirms the key and signature decode to the expected lengths', () => {
     expect(decodeBase64UrlStrict(RIQ_2026_PUBLIC_KEY, 32).length).toBe(32)
     expect(decodeBase64UrlStrict(SIGNATURE, 64).length).toBe(64)
   })
+
+  it('rejects a signature that decodes to 64 bytes from a non-canonical encoding', () => {
+    // Same 64 decoded bytes, but the final character carries non-zero
+    // padding bits, so the re-encode round trip differs from the input.
+    const nonCanonical = `${SIGNATURE.slice(0, -1)}${SIGNATURE.endsWith('g') ? 'h' : 'g'}`
+    expect(nonCanonical).not.toBe(SIGNATURE)
+    expect(() => decodeBase64UrlStrict(nonCanonical, 64)).toThrow(/canonical/)
+  })

🤖 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 83 - 86, Add a test case
alongside the existing decodeBase64UrlStrict tests that uses a non-canonical
base64url signature vector whose final character contains non-zero padding bits.
Assert that it is rejected by the re-encode round-trip validation while still
representing the expected decoded length, covering the branch distinct from
alphabet and length failures.

cli/src/verify.ts (1)

31-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the repeated read-and-wrap block.

The three read blocks differ only in the path, the encoding, and the label. A small helper removes the duplication and keeps the error text consistent if a fourth input appears later.

♻️ Proposed refactor

-  let tarballBytes: Buffer
-  let manifestJson: string
-  let signature: string
-  try {
-    tarballBytes = fsImpl.readFileSync(options.tarballPath)
-  } catch (err) {
-    throw new VerifyCommandError(
-      `could not read tarball '${options.tarballPath}': ${err instanceof Error ? err.message : String(err)}`,
-    )
-  }
-  try {
-    manifestJson = fsImpl.readFileSync(options.manifestPath, 'utf8')
-  } catch (err) {
-    throw new VerifyCommandError(
-      `could not read manifest '${options.manifestPath}': ${err instanceof Error ? err.message : String(err)}`,
-    )
-  }
-  try {
-    signature = fsImpl.readFileSync(options.signaturePath, 'utf8').trim()
-  } catch (err) {
-    throw new VerifyCommandError(
-      `could not read signature '${options.signaturePath}': ${err instanceof Error ? err.message : String(err)}`,
-    )
-  }
+  function read(label: string, filePath: string): Buffer
+  function read(label: string, filePath: string, encoding: 'utf8'): string
+  function read(label: string, filePath: string, encoding?: 'utf8'): Buffer | string {
+    try {
+      return encoding ? fsImpl.readFileSync(filePath, encoding) : fsImpl.readFileSync(filePath)
+    } catch (err) {
+      throw new VerifyCommandError(
+        `could not read ${label} '${filePath}': ${err instanceof Error ? err.message : String(err)}`,
+      )
+    }
+  }
+
+  const tarballBytes = read('tarball', options.tarballPath)
+  const manifestJson = read('manifest', options.manifestPath, 'utf8')
+  const signature = read('signature', options.signaturePath, 'utf8').trim()

🤖 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/verify.ts` around lines 31 - 54, Extract the repeated file-reading
and VerifyCommandError-wrapping logic from the tarball, manifest, and signature
reads into a small helper. Have the helper accept the file path, optional
encoding, and input label, preserve the existing error message format, and reuse
it at each call site while retaining signature trimming.

cli/src/verify-core.ts (1)

78-85: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Restrict the trust-store lookup to own string properties.

trustStore is a plain object literal, so it inherits from Object.prototype. A manifest that sets keyId to constructor, toString, or valueOf makes trustStore[manifest.keyId] return an inherited function. That value is truthy, so this untrusted-key guard does not fire, and the code passes a function into verifyManifestSignature.

Verification still fails, because decodeBase64UrlStrict coerces the value to a string and rejects it on the alphabet check. So this is not a bypass. The result is a wrong reason code: the operator sees bad-signature for a key the CLI never trusted, and the safety of the path depends on a coercion in a different module. cli/src/install.ts at Line 172 uses the same lookup shape and reports key presence the same way.

🛡️ Proposed hardening

-  const trustedKey = trustStore[manifest.keyId]
-  if (!trustedKey) {
+  const trustedKey = Object.hasOwn(trustStore, manifest.keyId)
+    ? trustStore[manifest.keyId]
+    : undefined
+  if (typeof trustedKey !== 'string' || trustedKey.length === 0) {
     return {
       ok: false,
       code: 'untrusted-key',
       message: `manifest was signed with keyId '${manifest.keyId}', which this CLI does not trust`,
     }
   }

🤖 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/verify-core.ts` around lines 78 - 85, Update the trustStore lookup in
the verification flow around trustedKey to accept only own properties whose
values are strings, using an own-property check before reading or validating the
stored key. Ensure inherited names such as constructor, toString, and valueOf
follow the existing untrusted-key response, and apply the same protection to the
corresponding key-presence lookup in install.ts.

tests/cli/verify-core.test.ts (1)

19-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a passing case through verifyArtifact.

Every case in this file asserts ok: false. No test asserts ok: true through verifyArtifact, because the real 88,107-byte tarball is not in the repository. tests/modules/artifact/vectors.test.ts covers the success path only at the verifyManifestSignature level, so it does not exercise how verifyArtifact wires the digest check, the size check, and the trust-store lookup into a success result. A wiring defect on that path would pass CI today.

You do not need the real artifact. Generate an ed25519 key pair in the test, sign a canonical manifest that describes a small local buffer, and pass a local trust store that holds the generated public key.

💚 Proposed positive-path test

+import { generateKeyPairSync, sign as cryptoSign } from 'node:crypto'
+import { canonicalizeManifest, sha256Hex } from '../../src/modules/artifact/index.js'
+
+it('returns ok for a locally signed manifest that matches the bytes', () => {
+  const { publicKey, privateKey } = generateKeyPairSync('ed25519')
+  const rawPublicKey = publicKey.export({ format: 'der', type: 'spki' }).subarray(-32)
+  const localTrustStore = { 'test-key': rawPublicKey.toString('base64url') }
+
+  const bytes = Buffer.from('not a real tarball, but real bytes')
+  const manifest = {
+    schema: 'helpthread-module-manifest/1' as const,
+    module: 'draft-assistant',
+    semver: '0.3.0',
+    artifactSha256: sha256Hex(bytes),
+    artifactBytes: bytes.length,
+    sourceRevision: '23230a9',
+    minEngineApi: '1.0.0',
+    builtAt: '2026-08-04T01:24:51.266Z',
+    keyId: 'test-key',
+  }
+  const canonical = canonicalizeManifest(manifest)
+  const signature = cryptoSign(null, Buffer.from(canonical, 'utf8'), privateKey).toString(
+    'base64url',
+  )
+
+  const result = verifyArtifact(bytes, canonical, signature, localTrustStore)
+  expect(result.ok).toBe(true)
+  if (result.ok) expect(result.manifest.module).toBe('draft-assistant')
+})

🤖 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 - 26, Add a positive-path test
in the verifyArtifact test suite that generates an Ed25519 key pair, signs a
canonical manifest describing a small local buffer, and builds a trust store
containing the generated public key. Pass the matching buffer, manifest,
signature, and trust store to verifyArtifact, then assert ok is true, covering
digest, size, signature, and trust-store wiring.

tests/cli/catalog.test.ts (1)

215-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for the MAX_ARTIFACT_BYTES ceiling.

The fetchTarball block covers success and a non-ok status, but not the size refusal, which is the security control in that function. A test that returns an oversized ArrayBuffer locks the limit in place.

💚 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/,
+    )
+  })

Import MAX_ARTIFACT_BYTES from ../../cli/src/catalog.js alongside the existing imports.

🤖 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
covering the MAX_ARTIFACT_BYTES ceiling, importing that constant alongside the
existing catalog imports. Have the fake fetch return an ArrayBuffer larger than
the limit with an otherwise successful response, and assert that fetchTarball
rejects with CatalogError.

cli/src/catalog.ts (2)

14-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider a request timeout for every catalog call.

FetchLike carries no abort signal, so fetchCatalog, requestDownloadUrl, and fetchTarball can each hang until the operating system gives up. The PR bounds artifact size but not time. Add an optional signal to FetchLike and pass an AbortSignal.timeout(...) from cli/src/main.ts.

Also applies to: 140-162

🤖 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 14 - 24, Update the FetchLike init type and
the catalog request functions fetchCatalog, requestDownloadUrl, and fetchTarball
to accept and forward an optional AbortSignal. In cli/src/main.ts, supply an
AbortSignal.timeout(...) for each catalog call so requests have an explicit
deadline while preserving the existing artifact-size limits and response
handling.

---

82-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

compareSemver ranks a prerelease above its own release.

'1.0.0-rc.1'.split('.') yields ['1','0','0-rc','1'], and Number.parseInt('0-rc', 10) is 0, so the string compares as 1.0.0.1 and wins against 1.0.0. If the catalog ever publishes a prerelease, an unpinned install selects it as "latest". The doc comment accepts malformed segments, so this is a hardening suggestion rather than a present defect: filter resolveVersion candidates to strict ^\d+\.\d+\.\d+$ versions, or compare with a semver library.

🤖 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 82 - 91, Harden latest-version selection so
prerelease strings cannot outrank stable releases: in the resolveVersion
candidate selection flow, filter versions using the strict /^\d+\.\d+\.\d+$/
shape before calling compareSemver, while preserving compareSemver’s
malformed-segment fallback behavior.

cli/src/install.ts (1)

121-148: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Nothing reads tmpTarballPath, so the temp write only spills paid bytes to disk.

safeExtract receives tarballBytes from memory on Line 212. tmpTarballPath is written on Line 125 and never read again. The write therefore adds a copy of the proprietary artifact on disk, and the try/finally block exists only to remove a file that need not be created. Remove the temp write and the cleanup, or read the extraction input from that path if an on-disk copy is intended.

♻️ Proposed simplification

   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,
+  })
 }

The node:os import becomes unused after this change.

🤖 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
artifact path and write in the installation flow around installVerifiedArtifact,
along with the surrounding try/finally cleanup that only removes that directory.
Continue passing the in-memory tarballBytes to installVerifiedArtifact and
remove the now-unused node:os import.

tests/cli/install.test.ts (1)

66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Isolate HELPTHREAD_LICENSE_KEY in these tests.

runInstall reads process.env.HELPTHREAD_LICENSE_KEY before it calls deps.getLicenseKey(). No test in this file sets or clears that variable. If a developer machine or CI job exports it, the license-key tests at Lines 218-274 and Lines 581-656 stop exercising what they claim: the injected secret is never used, so "the thrown error does not contain the key" passes for the wrong reason. The comment on Line 176, 'unused-because-env-var-set', also describes a variable that this file never sets.

Clear the variable in beforeEach and set it explicitly in any test that wants the env path.

💚 Proposed change

-import { afterEach, beforeEach, describe, expect, it } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'

 beforeEach(() => {
   workDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ht116-install-'))
+  // `runInstall` prefers this variable over `deps.getLicenseKey`; an
+  // inherited value would make the leak assertions vacuous.
+  vi.stubEnv('HELPTHREAD_LICENSE_KEY', '')
+  vi.stubEnv('HELPTHREAD_CATALOG_ORIGIN', '')
 })
 
 afterEach(() => {
+  vi.unstubAllEnvs()
   fs.rmSync(workDir, { recursive: true, force: true })
 })

Then change Line 176 to a plain getLicenseKey: async () => 'test-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/install.test.ts` around lines 66 - 74, Update the test setup around
beforeEach to clear process.env.HELPTHREAD_LICENSE_KEY before each test, and
explicitly set it only in tests that exercise the environment-variable path. In
the mock dependency near the existing “unused-because-env-var-set” comment,
remove that misleading comment and keep getLicenseKey returning the test license
key normally.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/src/catalog.ts (1)

234-249: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information

Reachability: External

Reachability path

● Entry
  cli/bin/helpthread-module.js
│
▼
● Hop
  cli/src/main.ts:20
  main
│
▼
● Sink
  cli/src/catalog.ts

Require HTTPS for every Module artifact-download hop.

The marketplace controls downloadUrl, and the current check permits any non-empty string before fetchTarball dereferences it. An http: URL exposes the Module artifact to a network observer. If fetchImpl follows redirects, an HTTPS URL can also downgrade without an adapter policy. The GET omits the license key, and signature checks preserve integrity, but neither control protects artifact confidentiality. Enforce HTTPS at fetchTarball, and reject redirects until redirect targets receive the same HTTPS validation. Add direct-HTTP and downgrade-redirect tests.

Also applies to: 291-292

🤖 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 234 - 249, Enforce HTTPS for every
artifact-download request in fetchTarball, including validating the initial
downloadUrl and rejecting redirects before following them so an HTTPS URL cannot
downgrade to HTTP. Preserve the existing malformed-response validation in the
download flow, and add coverage for direct HTTP URLs and HTTPS-to-HTTP redirect
attempts.

src/modules/artifact/manifest.ts (1)

197-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Section header sits below the code it describes.

The header at Line 197 announces signature verification, but verifyManifestSignature is defined at Lines 95-122. Only decodeBase64UrlStrict follows the header. Move the header above Line 85, or rename it to // --- base64url decoding ---.

🤖 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/manifest.ts` at line 197, Align the section header in
manifest.ts with the code it describes: move the “signature verification” header
above verifyManifestSignature, or rename the existing header to identify the
decodeBase64UrlStrict section.

tests/modules/artifact/vectors.test.ts (2)

83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a vector for the non-canonical base64url branch.

decodeBase64UrlStrict in src/modules/artifact/manifest.ts performs three checks: alphabet, exact decoded length, and a re-encode round trip. This suite covers the alphabet check at Lines 66-72 and the length check at Lines 83-86. The round-trip check has no test. That branch is the one that rejects a signature whose trailing unused bits are non-zero, so it protects against signature malleability. Add one case that asserts a rejection there.

🧪 Proposed additional vector

   it('decodeBase64UrlStrict independently confirms the key and signature decode to the expected lengths', () => {
     expect(decodeBase64UrlStrict(RIQ_2026_PUBLIC_KEY, 32).length).toBe(32)
     expect(decodeBase64UrlStrict(SIGNATURE, 64).length).toBe(64)
   })
+
+  it('rejects a non-canonical base64url encoding of the real signature', () => {
+    // Same 64 bytes, but the final character sets unused trailing bits,
+    // so Buffer re-encodes it to a different string.
+    const raw = decodeBase64UrlStrict(SIGNATURE, 64)
+    const nonCanonical = `${SIGNATURE.slice(0, -1)}${SIGNATURE.endsWith('g') ? 'h' : 'g'}`
+    expect(Buffer.from(nonCanonical, 'base64url').equals(raw)).toBe(true)
+    expect(nonCanonical).not.toBe(SIGNATURE)
+    expect(() => decodeBase64UrlStrict(nonCanonical, 64)).toThrow(/canonical/)
+    expect(verifyManifestSignature(MANIFEST_JSON, nonCanonical, RIQ_2026_PUBLIC_KEY)).toBe(false)
+  })

Confirm the substituted character produces the same bytes before you commit the case; adjust it if the assertion at the top of the test fails.

🤖 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 83 - 86, Add a test case
in the decodeBase64UrlStrict tests that uses a base64url string with non-zero
trailing unused bits while preserving the expected decoded length, then assert
it is rejected by the re-encode round-trip validation. Keep the existing
alphabet and length vectors unchanged, and verify the substituted character
still decodes to the same bytes before finalizing the vector.

---

30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix the inaccurate comment on UNRELATED_PUBLIC_KEY.

The comment says "32 zero bytes re-keyed via a distinct value". That describes nothing concrete, and the value is not derived from zero bytes. State the real provenance: an independently generated Ed25519 public key that is unrelated to riq-2026.

📝 Proposed comment fix

-/** A different, unrelated valid ed25519 public key (32 zero bytes re-keyed via a distinct value) — used to prove the verifier is actually checking the key, not just "some" signature shape. Generated once, not derived from anything secret. */
+/** A second, independently generated ed25519 public key, unrelated to `riq-2026` — used to prove the verifier checks the specific key, not just "some" valid signature shape. Its private half is not retained anywhere. */

🤖 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 30 - 31, Update the
documentation comment for UNRELATED_PUBLIC_KEY to state that it is an
independently generated Ed25519 public key unrelated to riq-2026, removing the
inaccurate description about zero bytes and re-keying.

tests/modules/artifact/module-config.test.ts (1)

8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The fixture is not byte-for-byte.

The comment claims the fixture matches the shipped module.config.json byte-for-byte. JSON.stringify on an object literal produces the key order and spacing written here, not the bytes of the released file. parseModuleConfig is insensitive to both, so the tests remain valid. Reword the comment to say the fixture reproduces the shipped content, or embed the literal string as tests/modules/artifact/vectors.test.ts does at its Line 23.

🤖 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
REAL_DRAFT_ASSISTANT_CONFIG comment to state that the fixture reproduces the
shipped module.config.json content rather than matching it byte-for-byte, unless
replacing the JSON.stringify fixture with the exact literal file contents is
preferred.

src/modules/artifact/module-config.ts (1)

57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider sharing the validation scaffolding with manifest.ts.

src/modules/artifact/manifest.ts and this file each implement the same four checks: JSON-object top level, unknown-key rejection, missing-key rejection, and non-empty-string validation. The error text differs only by a prefix. Extract the helpers into an internal module, for example src/modules/artifact/parse-util.ts, and give it a message prefix parameter. Both parsers then share one strictness policy, so a future fix applies to both schemas.

🤖 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 57 - 58, Extract the
duplicated validation helpers used by the module-config and manifest parsers
into a shared internal utility, such as parse-util.ts, parameterized by the
error-message prefix. Update both parsers to reuse it for top-level object
checks, unknown-key rejection, missing-key rejection, and non-empty-string
validation while preserving their existing schema-specific keys and error
prefixes.

cli/src/verify-core.ts (1)

61-76: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider checking the byte length before the SHA-256.

The size comparison is constant time. The hash walks the whole buffer. Checking artifactBytes first rejects a wrong-size tarball without hashing it. The download path already caps the tarball size, so the current order is not a real cost. If you reorder, update the doc comment at Lines 41-45 and the expected codes in tests/cli/verify-core.test.ts, which relies on the documented order.

🤖 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/verify-core.ts` around lines 61 - 76, The verify flow should check
tarball byte length before computing SHA-256 to reject size mismatches without
hashing. Reorder the checks in the verification function, update the nearby doc
comment describing validation order, and adjust the expected error-code order in
verify-core tests to match.

tests/cli/verify-core.test.ts (2)

19-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No test asserts the success path of verifyArtifact.

All five cases assert result.ok === false. An implementation that always returned a failure would pass this whole suite. The success path is the one that gates installation, and it is the only path that exercises canonicalizeManifest, the TRUST_STORE hit, and the manifest field on VerifyOk.

The real 88,107-byte tarball is not committed, so add the case with generated material instead: create a small buffer, build a manifest that describes its real SHA-256 and length, sign it with a locally generated Ed25519 key, and pass a local trust store. Add a separate assertion that TRUST_STORE contains the riq-2026 entry, so the pinned key stays covered.

🤖 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 - 26, Add a successful
verifyArtifact test using a generated small buffer, a manifest containing its
actual SHA-256 and length, a locally generated Ed25519 signature, and a matching
local trust store; assert result.ok is true and validate the returned manifest.
Also add a separate assertion that TRUST_STORE includes the riq-2026 entry,
while preserving the existing failure cases.

---

50-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use computed digest values for the buffer vectors.

The pinned digests are correct, but lines 37, 61, and 81 still hard-code artifactSha256 values for generated empty and zero-filled buffers. Define a test-local helper such as sha256Hex(Buffer.alloc(length)) so future edits cannot drift from the buffer length without failing the test.

🤖 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 50 - 71, Replace the hard-coded
artifactSha256 values for generated empty and zero-filled buffers in the
affected tests with values computed by a test-local sha256Hex helper applied to
Buffer.alloc(length). Update each relevant manifest construction, including the
untrustedKeyManifest case, while preserving the existing buffer lengths and
assertions.

cli/src/verify.ts (1)

31-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse the three read blocks into one helper.

The three try/catch blocks differ only by the path, the label, and the encoding. A single helper removes the repetition and keeps the error text uniform.

♻️ Proposed refactor

-  let tarballBytes: Buffer
-  let manifestJson: string
-  let signature: string
-  try {
-    tarballBytes = fsImpl.readFileSync(options.tarballPath)
-  } catch (err) {
-    throw new VerifyCommandError(
-      `could not read tarball '${options.tarballPath}': ${err instanceof Error ? err.message : String(err)}`,
-    )
-  }
-  try {
-    manifestJson = fsImpl.readFileSync(options.manifestPath, 'utf8')
-  } catch (err) {
-    throw new VerifyCommandError(
-      `could not read manifest '${options.manifestPath}': ${err instanceof Error ? err.message : String(err)}`,
-    )
-  }
-  try {
-    signature = fsImpl.readFileSync(options.signaturePath, 'utf8').trim()
-  } catch (err) {
-    throw new VerifyCommandError(
-      `could not read signature '${options.signaturePath}': ${err instanceof Error ? err.message : String(err)}`,
-    )
-  }
+  function read(label: string, filePath: string): Buffer {
+    try {
+      return fsImpl.readFileSync(filePath)
+    } catch (err) {
+      throw new VerifyCommandError(
+        `could not read ${label} '${filePath}': ${err instanceof Error ? err.message : String(err)}`,
+      )
+    }
+  }
+
+  const tarballBytes = read('tarball', options.tarballPath)
+  const manifestJson = read('manifest', options.manifestPath).toString('utf8')
+  const signature = read('signature', options.signaturePath).toString('utf8').trim()

🤖 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/verify.ts` around lines 31 - 54, Extract the repeated file-reading
and VerifyCommandError handling into one local helper near the verification
flow, accepting the file path, descriptive label, and optional encoding. Use
this helper to initialize tarballBytes, manifestJson, and signature, preserving
signature trimming and the existing uniform error messages.

cli/src/install.ts (1)

137-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the unused temp-file write.

Line 141 writes the full artifact to tmpTarballPath, but nothing reads it. installVerifiedArtifact receives tarballBytes, and safeExtract also takes the buffer. The write only adds a full-size disk write per install and keeps the temp directory (and its finally cleanup) alive for no consumer. If a later step needs the file on disk, keep it and add that consumer; otherwise drop the temp directory entirely.

♻️ Proposed simplification

   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)
-
-  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,
+  })
 }

The node:os import at Line 11 then becomes unused.

🤖 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 137 - 164, Remove the unused
temporary-directory and tarball-file creation around installVerifiedArtifact,
including the writeFileSync call and related path/os usage. Preserve passing
tarballBytes to installVerifiedArtifact and remove the now-unnecessary finally
cleanup block and node:os import.

tests/cli/install.test.ts (1)

733-821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the positive case for $HELPTHREAD_LICENSE_KEY.

Both tests here cover the "absent" side of the check at cli/src/install.ts lines 82-84. No test covers the "present" side, so a regression that always prompts would still pass. Add a case that sets a non-empty value and asserts getLicenseKey is not called.

💚 Proposed test

+  it('uses the env var and does not prompt when it holds a value', async () => {
+    process.env.HELPTHREAD_LICENSE_KEY = 'env-supplied-key'
+    const fixture = buildSignedFixture({ moduleSlug: 'fixture-module', version: '1.0.0' })
+    const fetchImpl = fakeFetch({
+      'https://catalog.example/api/v1/modules': () => ({
+        ok: true,
+        status: 200,
+        json: fixture.catalog,
+      }),
+      'https://catalog.example/api/v1/download': () => ({
+        ok: false,
+        status: 401,
+        json: { error: { code: 'unauthorized' } },
+      }),
+    })
+    let promptCalled = false
+    await expect(
+      runInstall(
+        {
+          help: false,
+          moduleSlug: 'fixture-module',
+          catalogOrigin: 'https://catalog.example',
+          version: undefined,
+          dir: path.join(workDir, 'env-key'),
+          force: false,
+        },
+        {
+          fetchImpl,
+          getLicenseKey: async () => {
+            promptCalled = true
+            return 'k'
+          },
+          log: () => {},
+        },
+      ),
+    ).rejects.toThrow(/download request failed/)
+    expect(promptCalled).toBe(false)
+  })

🤖 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/install.test.ts` around lines 733 - 821, Add a positive-case test
alongside the existing empty and whitespace-only cases in the runInstall test
suite: set HELPTHREAD_LICENSE_KEY to a non-empty value, provide the required
catalog/download fixtures, and assert the injected getLicenseKey callback is not
called while preserving the expected successful or download-path outcome.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes