You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
generate evidence-backed bilingual CLI Release Notes with local coverage/language/readability gates, inspectable quality reports, and at most three Doc Agent repair attempts
isolate reusable pre/post-merge dry runs to contents: read with no persisted Git credentials, npm token, OSS credentials, or publish commands
persist a durable release commit before external writes and reconcile npm gitHead, immutable tags, GitHub Release inventory, and version PR state
publish a versioned four-platform runtime asset contract and verify HTTPS downloads with SHA-256 checksums
Validation
28 Release Notes/workflow Node tests
5 npm wrapper tests
3 release asset contract Python tests
42 CLI Python tests in an isolated Python 3.12 environment
npm pack --dry-run --json with an isolated cache
actionlint and git diff --check
good and bad local inspection runs; bad manual notes retain evidence.json and quality-report.json while failing closed
Safety / migration
no npm publish, OSS upload, tag, GitHub Release, MemOS-Docs PR, or deployment was performed during this work
automatic npm-only binary metadata recovery is intentionally disabled
before the next live release, separately authorize and backfill audited baseline tag v1.0.6 at npm gitHeadc18ced54beeb817f6d3f0def1d43eca66da94817, or keep the migration-only baseline variable until that is done
When matches.length === 0 the function returns early with state: 'absent', which is correct. However, flattenReleasePages uses value.every(item => Array.isArray(item)) to detect whether the input is a list-of-pages or a single page. If the API ever returns a single-item page (an array containing exactly one release object), every will return false because the item is not an array, so it will be treated correctly as a single page. But if pages is passed as [[release1]] (the real format used in every workflow call), a page with one release object will be classified correctly too. The edge case that is NOT covered is pages: [[]] — an empty first page — which is tested, versus pages: [] (no pages at all) — also tested. What is NOT tested is flattenReleasePages([[]]) with a zero-length inner array: the function returns [], and matches.length === 0, so this path is exercised, but there is no explicit test asserting that pages: [[]] with requireExisting: false returns state: 'absent' with ok: true. Additionally, flattenReleasePages has no test for a page containing a mix of valid objects and nulls (the .filter(item => item && typeof item === 'object') guard).
The source-ID check requires sourceIds.length === 1, but the regex in docAgentSourceIds uses matchAll and will match every <!-- doc-agent: source-id=... --> comment in the body. If a release body legitimately repeats the comment (e.g., from a copy-paste in manual notes or a template), the check will fail even though the correct source ID is present. ensureSourceHint in draft-cli-release-notes.mjs guards against double-insertion when the string 'doc-agent: source-id=' already appears, but that guard only applies to machine-generated notes. Manual notes that already carry the marker could still result in multiple matches. Consider changing the check to !sourceIds.includes(expectedSourceId) and separately flagging sourceIds.length > 1 as a warning rather than an error, or tightening ensureSourceHint to also de-duplicate on the regex level.
💡 Suggested Change
Before:
if (expectedSourceId && (sourceIds.length !== 1 || sourceIds[0] !== expectedSourceId)) {
errors.push(
`GitHub Release ${releaseTag} must contain exactly one Doc Agent source id ${expectedSourceId}; found ${sourceIds.join(", ") || "none"}`,
);
}
After:
if (expectedSourceId && !sourceIds.includes(expectedSourceId)) {
errors.push(
`GitHub Release ${releaseTag} must contain a Doc Agent source id ${expectedSourceId}; found ${sourceIds.join(", ") || "none"}`,
);
}
These negative assertions check for the literal substrings NPM_TOKEN and OSS_ACCESS_KEY, but the dry-run workflow already accepts DOC_AGENT_RELEASE_NOTES_DRAFT_TOKEN (which does not contain either substring). If a future secrets block added an alias like NPM_PUBLISH_TOKEN instead of NPM_TOKEN, or OSS_KEY_ID instead of OSS_ACCESS_KEY, the test would pass while credentials were accessible. Consider also asserting against broader patterns such as /NODE_AUTH_TOKEN/, /OSS_ACCESS_KEY_ID/, and /OSS_ACCESS_KEY_SECRET/ to close the alias gap — or enumerate every secrets. reference the dry-run workflow is permitted to declare.
Using String.indexOf on raw YAML to assert step execution order is fragile. These strings are step name: / shell-run substrings that happen to appear in file order today, but file order does not always equal execution order in GitHub Actions — jobs with the same needs: chain can run in any order, and two steps in different jobs might have a positional ordering in the file that contradicts their actual dependency graph. Additionally, if any of these strings is ever split across a YAML block scalar or moved to a job comment, the check can give a false positive. Consider asserting that both oss and npm indices are greater than durableand that all three are >= 0 separately so a missing string produces a clear failure rather than a vacuously-true comparison (-1 < -1 is false, but -1 < 0 for oss or npm would compare correctly only by coincidence).
💡 Suggested Change
Before:
const durable = releaseWorkflow.indexOf("Push a durable release source before external publication");
const oss = releaseWorkflow.indexOf("Upload assets to OSS and verify each object");
const npm = releaseWorkflow.indexOf('npm publish --access public --tag "${NPM_DIST_TAG}"');
assert.ok(durable >= 0 && durable < oss);
assert.ok(durable < npm);
After:
const durable = releaseWorkflow.indexOf("Push a durable release source before external publication");
const oss = releaseWorkflow.indexOf("Upload assets to OSS and verify each object");
const npm = releaseWorkflow.indexOf('npm publish --access public --tag "${NPM_DIST_TAG}"');
assert.ok(durable >= 0, "durable source step not found");
assert.ok(oss >= 0, "OSS upload step not found");
assert.ok(npm >= 0, "npm publish step not found");
assert.ok(durable < oss, "durable source must precede OSS upload");
assert.ok(durable < npm, "durable source must precede npm publish");
This regex only verifies that a permissions: contents: read block exists somewhere in the dry-run workflow YAML, but does not confirm it is at the top-level workflow scope. The dry-run workflow does have a top-level permissions block, but if it were ever moved to only a single job block while another job inherited a broader default, this assertion would still pass. Consider anchoring the check to the workflow-level position — for example, by asserting the pattern appears before the first jobs: keyword, or by verifying that every job either omits a job-level permissions: block (inheriting the top-level read) or explicitly declares contents: read.
These three assertions confirm the presence of the inventory validator script and idempotency error string, but do not verify that the validator runs beforegh release create. In release.yml, the validator runs as part of a pre-publish reconciliation step and then again inside the gh release create error-handling block. If a future refactor moves the pre-create check to a later position, this test would not catch the regression. Add an indexOf-based ordering assertion (similar to the durable-source check above) to ensure validate-github-release-inventory.mjs appears before the gh release create invocation.
💡 Suggested Change
Before:
assert.match(releaseWorkflow, /validate-github-release-inventory\.mjs/);
assert.match(releaseWorkflow, /Refusing to issue a second create request/);
assert.doesNotMatch(releaseWorkflow, /gh release view/);
After:
assert.match(releaseWorkflow, /gh api --paginate --slurp/);
assert.match(releaseWorkflow, /validate-github-release-inventory\.mjs/);
assert.match(releaseWorkflow, /Refusing to issue a second create request/);
assert.doesNotMatch(releaseWorkflow, /gh release view/);
const inventoryCheck = releaseWorkflow.indexOf("validate-github-release-inventory.mjs");
const releaseCreate = releaseWorkflow.indexOf("gh release create");
assert.ok(inventoryCheck >= 0 && releaseCreate >= 0 && inventoryCheck < releaseCreate,
"inventory validation must precede gh release create");
Both readFileSync calls execute at module load time, outside any test() block. If either workflow file is absent (e.g., in a fork, a shallow clone, or a CI environment that does not check out .github/workflows/), the entire module throws an uncaught exception before Node's test runner can register any test, producing an opaque crash rather than a named test failure. Wrap the reads in a helper that throws a descriptive error, or move them inside a before/setup hook so the test runner can report the failure with context.
This assertion anchors to the error-message string npm records gitHead inside a shell echo statement. While this is marginally more stable than a YAML comment, it is still cosmetic text that a maintainer might rephrase (e.g., "npm published gitHead" or "gitHead mismatch") without changing the underlying logic. A more durable anchor would be to assert the actual shell variable check that enforces the postcondition, such as the conditional $\{npm_git_head\} != $\{EXPECTED_RELEASE_COMMIT\} or the env variable name EXPECTED_RELEASE_COMMIT.
💡 Suggested Change
Before:
assert.match(releaseWorkflow, /npm records gitHead/);
After:
assert.match(releaseWorkflow, /EXPECTED_RELEASE_COMMIT: \$\{\{ needs\.metadata\.outputs\.release_commit_sha \}\}/);
// Verify the postcondition enforcement expression, not the human-readable error message
assert.match(releaseWorkflow, /npm_git_head.*EXPECTED_RELEASE_COMMIT|EXPECTED_RELEASE_COMMIT.*npm_git_head/);
When EXPECTED_RELEASE_DRAFT or EXPECTED_RELEASE_PRERELEASE are missing or have an unexpected value, parseBoolean throws synchronously before main() writes any JSON to stdout. Any downstream workflow step that parses stdout as JSON will receive empty output and may fail silently or with an unhelpful parse error. This is inconsistent with how the rest of main() accumulates errors in the report.errors array. Consider wrapping the parseBoolean calls inside main() in a try/catch that emits a structured {ok: false, errors: [...]} JSON to stdout before exiting, matching the behavior of the inspectReleaseInventory path.
💡 Suggested Change
Before:
function parseBoolean(name) {
const value = required(name);
if (!["true", "false"].includes(value)) throw new Error(`${name} must be true or false`);
return value === "true";
}
Setting REQUIRED_DOC_AGENT_SOURCE_ID to an empty string silently falls back to DOC_AGENT_SOURCE_ID instead of disabling the source-ID check. This is inconsistent with how inspectReleaseInventory itself works: the library function supports requiredSourceId: "" to skip the check (line 80: if (expectedSourceId && ...)), but the CLI entry point makes that impossible. If a caller ever needs to invoke this script without the source-ID check, there is no way to express that intent via the CLI. The discrepancy should either be documented explicitly or the fallback should be removed so that REQUIRED_DOC_AGENT_SOURCE_ID= is treated as "skip the check" consistently with the module API.
The hardcoded delimiter __DOC_AGENT_EOF__ is never checked for collision against the written value. If any value — including draft_confidence (sourced from the external Doc Agent response) or previous_tag (a git tag) — contains the literal string __DOC_AGENT_EOF__ on its own line, the multiline block ends early and trailing content is parsed as GitHub Actions workflow commands, enabling output injection. Generate a unique random delimiter per call instead.
💡 Suggested Change
Before:
function appendOutput(name, value) {
if (!process.env.GITHUB_OUTPUT) return;
writeFileSync(process.env.GITHUB_OUTPUT, `${name}<<__DOC_AGENT_EOF__\n${value}\n__DOC_AGENT_EOF__\n`, {
flag: "a",
});
}
After:
function appendOutput(name, value) {
if (!process.env.GITHUB_OUTPUT) return;
const delimiter = `DOC_AGENT_EOF_${Math.random().toString(36).slice(2)}`;
writeFileSync(process.env.GITHUB_OUTPUT, `${name}<<${delimiter}\n${value}\n${delimiter}\n`, { flag: "a" });
}
reportFailure is awaited without a try/catch here. If the failure-report endpoint is down or returns a non-2xx status it throws, replacing the original draft error with a confusing failure-report error and losing the original diagnostic context. reportValidationFailureIfExhausted wraps the same call in try/catch — apply the same pattern here.
The warning is suppressed when local tags already exist. A network failure during a real release will silently proceed with stale local tags, potentially selecting the wrong previous tag, with no CI-visible signal. Emit the warning unconditionally.
💡 Suggested Change
Before:
} catch {
if (localTagsBeforeFetch.length === 0) {
warn("Failed to fetch tags; using local tags.");
}
}
After:
} catch {
warn("Failed to fetch tags from origin; continuing with local tags (results may be stale).");
}
The test only asserts that exactly 4 errors are returned, without checking what each error says. The implementation generates one error per failing condition (draft, prerelease, target_commitish, source-id). If a future refactor merges two conditions into one error message or splits one into two, the count might still be 4 while individual checks silently regress.
Consider asserting on specific error content for each condition, for example using assert.match or assert.ok(report.errors.some(e => /draft/.test(e))) for each expected failure.
There is no test for the case where a release body has no doc-agent comment at all (as distinct from a wrong source-id). The implementation checks sourceIds.length !== 1 || sourceIds[0] !== expectedSourceId, so an empty body produces sourceIds = [] and the error message says found none. This is a distinct error path from the wrong-source-id case already tested on line 50, and it is currently uncovered.
Add a test case with body: "" or body: "## No routing comment" to cover this boundary.
The created_at value happens to match today's date (2026-07-27), but created_at is not used in any validation logic — it is only stored in releaseSummary. Using today's date may mislead future maintainers into thinking this field has date-sensitive meaning in the tests. A clearly arbitrary, past date (e.g. "2025-01-01T00:00:00Z") would better signal that this value is just a placeholder.
There is no test for docAgentSourceIds when a body contains multiple doc-agent source-id comments. The function is named in the plural (sourceIds) and returns an array. The production validator enforces sourceIds.length !== 1 — meaning two routing comments also triggers an error — but this path is completely untested. A test with a body containing two <!-- doc-agent: source-id=... --> comments would close this gap.
The success-case confirmation string 'PUBLISH v1.0.7' is hardcoded here rather than composed via the already-imported expectedReleaseConfirmation('1.0.7'). If the phrase format ever changes (e.g., RELEASE instead of PUBLISH), this assertion will silently become stale — it would pass on the old format and fail to catch the regression.
Suggestion: derive the expected string from the helper:
The version and baseline_ref are hardcoded to fixed values ("1.0.7" and "c18ced5"). Unlike the pre-merge variant which uses these as static test fixtures (acceptable since it runs against in-PR code), this workflow runs post-merge on main and is supposed to keep validating the current release readiness. When the project bumps to v1.0.8, the dry run will silently continue validating the wrong version, potentially masking breakage in the actual next release. Consider reading the version from package.json dynamically (e.g., with a node -p "require('./package.json').version" step) and deriving baseline_ref from the latest git tag rather than hardcoding them.
No concurrency group is defined. Because this workflow triggers on every push to main that touches the listed paths, rapid successive pushes will queue multiple runs simultaneously, each uploading an artifact with the same name (memos-cloud-cli-post-merge-release-inspection). Add a concurrency block with cancel-in-progress: true so that only the latest run proceeds, avoiding wasted runner minutes and artifact conflicts.
No timeout-minutes is set on the inspect job. The called reusable workflow (release-dry-run.yml) runs multi-platform builds, npm installs, Python tests, and an external Doc-Agent HTTP call, any of which can hang indefinitely. Add timeout-minutes (e.g., 60) to bound the maximum runner consumption per run.
Fail-open default: when dryRun is undefined (e.g. DRY_RUN env var is absent or the script is called from a new workflow step that omits it), this defaults to "true" and the confirmation gate is silently skipped — allowing a real publish to proceed without any human check. For a security gate, the safe default is fail-closed (treat an absent value as false, i.e. require confirmation). The primary release.yml always sets DRY_RUN, but any future caller that omits it will get the wrong behavior with no warning.
Suggestion: default to "false" so a missing env var forces confirmation rather than bypassing it:
The baseline_ref is a 7-character short SHA. Short SHAs are ambiguous and can collide as the repository grows — a future commit could share the same prefix, causing git to resolve baseline_ref to an unintended commit. This would silently shift the diff baseline in the dry-run validation, potentially allowing a bad release to pass the pre-merge check. Use the full 40-character SHA instead.
The same issue exists in post-merge-release-dry-run.yml at the same input, so both files should be updated together.
The artifact name is static and not scoped to the triggering branch. Because this workflow fires on push to any docs-sync/** branch and there is no concurrency group defined, simultaneous runs from different branches (or rapid successive pushes to the same branch) will upload artifacts under the same name. GitHub Actions artifact names must be unique per workflow run but are displayed by name; overlapping runs can cause confusion, and if the reusable workflow ever adds retention/overwrite logic, this becomes a corruption risk.
Add a concurrency group at the workflow level and make the artifact name branch-scoped:
The version and baseline_ref inputs are hardcoded to a fixed release value. As the project advances, these will silently drift from the version actually being prepared, meaning the pre-merge gate validates a past release rather than the current one. There is no automated mechanism to keep them in sync with package.json.
Consider reading the version dynamically from package.json in a preceding step and passing it as an output, or add a required manual update checklist to the PR template that references these values explicitly.
Neither this workflow nor any job in the called release-dry-run.yml specifies timeout-minutes. GitHub's default is 6 hours. If the reusable workflow stalls (e.g., on a hung asset build, an unresponsive external endpoint, or a network issue during dependency installation), this pre-merge check will silently block the branch for up to 6 hours without alerting. Since this is a pre-merge gate, a stuck run directly blocks developer merges.
Add a timeout-minutes at the job level in the reusable workflow, and optionally at the caller level as well:
Restoring process.env by reassigning to a plain-object copy does not reliably reset the environment in all Node.js runtimes. process.env is a special live object; replacing the reference with a shallow copy ({ ...process.env }) means the reference in process.env now points to a plain object, but the actual native environment bindings are not rolled back. This pattern appears in three async tests (lines 212, 316, 343). If any assertion between Object.assign and the finally block throws before cleanup, follow-on tests may see leaked env vars (e.g. DOC_AGENT_RELEASE_NOTES_DRAFT_TOKEN) that cause false positives or negatives.
Use delete process.env[key] / process.env[key] = previous[key] for keys that were mutated, or use a helper that saves/restores individual keys:
This assertion tests resolvePreviousRef("1.0.0-beta.20", ...) but the git repo only contains tags up to v1.0.0-beta.19. There is no commit or tag for v1.0.0-beta.20, so the function is asked to resolve a version that does not exist in the test repo. If resolvePreviousRef consults the real git history for the current working directory (which is changed to the temp dir), the result is only well-defined if the function falls back to SemVer comparison against available tags. The test should document this assumption or, better, add a matching tag to make the precondition explicit:
The test for overly-long bullets asserts readability issues for both text_cn and text_en, but draft.validation_report.issues is only checked for text_en. There is no corresponding assertion for text_cn in validation_report.issues. If the implementation accidentally omits the text_cn validation-report entry, this test will pass silently — creating a coverage gap for the Chinese-text path.
The quality-report test supplies targetVersion: "1.0.7" and currentTag: "v1.0.7" to qualityReportFromDraft, but draft was produced by manualDraftFromEvidence(cliEvidence, ...) where cliEvidence uses current_tag: "v1.0.6". The mismatched version strings (1.0.7 vs 1.0.6) are not flagged by any assertion in this test, so if qualityReportFromDraft is expected to cross-validate them, a regression in that check would go undetected. Either align the versions or add a comment explaining the intentional mismatch.
The mock response helper only exposes .text(), but real fetch responses also provide .json(). If the implementation ever calls response.json() instead of response.text() (which is a common refactoring), every test using this mock will throw a TypeError: response.json is not a function that looks unrelated to the logic being tested. Adding a json() method keeps the mock compatible with both calling conventions:
The env block for this step (lines 74–80) maps inputs.version, inputs.tag, inputs.dry_run, inputs.publish_confirmation, inputs.git_ref, and inputs.default_branch — but never maps inputs.recover_existing_npm_release to RECOVER_EXISTING_NPM_RELEASE. The variable is therefore always unset in the shell, so this guard is permanently bypassed regardless of the input value. With set -u active, referencing an unset variable would also abort the script entirely before reaching this check. Add RECOVER_EXISTING_NPM_RELEASE: ${{ inputs.recover_existing_npm_release }} to the step's env: block.
💡 Suggested Change
Before:
if [ "${RECOVER_EXISTING_NPM_RELEASE}" = "true" ]; then
echo "::error::Automatic npm-only recovery is disabled for CLI binaries. Backfill an audited baseline tag from npm gitHead instead of rebuilding historical assets from a different source tree."
exit 1
fi
In GitHub Actions expression context, inputs.dry_run is typed as boolean but when compared with != true inside ${{ }} the result may not evaluate as expected. More critically, the same persist-credentials pattern is repeated in the build job (line 375) and assemble-and-publish job (line 414) — those jobs do not perform any git push themselves, so there is no reason for them to persist credentials at all. Unnecessary credential persistence on build runners (especially macOS and Windows) increases the attack surface if a build step is compromised. For build and assemble-and-publish, always set persist-credentials: false; only the metadata job needs it conditionally.
The version value from needs.metadata.outputs.version is interpolated directly into the run: shell command via ${{ }} rather than being passed through an env: variable. Although the version is validated as semver in the metadata job, direct expression interpolation into shell is an injection anti-pattern per the review checklist. If the upstream validation were ever loosened or bypassed, shell-special characters in the version string (e.g. a backtick or $(...)) would be interpreted by the shell. The same pattern appears in Verify target archive name (line 398), the assemble-and-publishValidate the complete OSS/npm asset contract step (lines 441–446), and the artifact path (line 403). Pass through env: instead.
💡 Suggested Change
Before:
- name: Synchronize version
run: node scripts/sync-version.js "${{ needs.metadata.outputs.version }}"
macos-15-intel is not a documented GitHub-hosted runner label. GitHub offers macos-13 for Intel-based runners; macos-14 and macos-15 are Apple Silicon (ARM). Using an unrecognized or unavailable label will cause the job to queue indefinitely or fail immediately. Verify the intended label — for Intel x64 macOS, macos-13 is the standard choice.
When git rev-parse "${remote_branch_sha}^" fails (e.g. the commit is an orphan with no parent), the || true suppression leaves remote_parent_sha empty. The subsequent check [ "${remote_parent_sha}" != "${source_sha}" ] is then [ "" != "<sha>" ], which evaluates to true. Combined with remote_branch_sha != source_sha also being true, the workflow incorrectly aborts with "refusing to reuse or overwrite" for any orphan-commit release branch. Add an explicit empty-string guard before the comparison.
💡 Suggested Change
Before:
remote_parent_sha="$(git rev-parse "${remote_branch_sha}^" 2>/dev/null || true)"
if [ "${remote_branch_sha}" != "${source_sha}" ] && [ "${remote_parent_sha}" != "${source_sha}" ]; then
The oss2 dependency is installed inline with a loose version range (>=2.19,<3) without a lockfile or pinned hash. Every release run will fetch whatever is the latest compatible version from PyPI at that moment, making OSS uploads non-reproducible. A newly published oss2 patch with a regression could silently break production releases. Pin to an exact version (e.g. oss2==2.19.0) or add it to a pinned requirements-release.txt and use pip install -r requirements-release.txt.
💡 Suggested Change
Before:
python -m pip install 'oss2>=2.19,<3'
After:
python -m pip install 'oss2==2.19.0'
38. .github/workflows/release.yml (L44-L46)
No timeout-minutes is set on any of the three jobs (metadata, build, assemble-and-publish). The build job runs on paid macOS runners and performs network I/O (pip/npm installs, binary builds). The assemble-and-publish job contains retry loops with sleep calls for OSS upload, npm publish, tag push, and GitHub Release creation — a single hung network call could consume runner-minutes indefinitely. Add a timeout-minutes to each job (e.g. 30 for metadata, 60 for build, 30 for assemble-and-publish).
The outer guard at the top of download uses redirectCount > MAX_REDIRECTS (strict greater-than), but the inner guard here uses redirectCount >= MAX_REDIRECTS. This off-by-one inconsistency means the inner check fires one redirect earlier than intended, making the effective limit MAX_REDIRECTS - 1 (i.e., 4 redirects instead of 5) when the response actually carries a Location header. Remove the inner duplicate check and rely solely on the top-level guard.
The response.resume() call on the redirect path discards the response body without attaching an error handler to the response stream. If a network error occurs while the response is being drained, Node.js will emit an unhandled 'error' event on the response stream, which — since there is no listener — will crash the process. Attach response.on('error', reject) immediately after response.resume().
💡 Suggested Change
Before:
response.resume();
if (redirectCount >= MAX_REDIRECTS) {
After:
response.resume();
response.on("error", reject);
if (redirectCount > MAX_REDIRECTS) {
41. scripts/postinstall.js (L148-L151)
Attaching a 'data' listener on a Readable that is also passed to pipe() is unsafe. In Node.js streams, adding a 'data' listener switches the stream into flowing mode independently of the pipe destination. Under backpressure, pipe may pause the source, but the 'data' listener can still receive (or miss) chunks depending on buffering, leading to the hash being computed over a different byte sequence than what was written to disk. Use a crypto.createHash Transform stream inserted into the pipe chain instead:
consthash=crypto.createHash('sha256');consthashStream=newcrypto.Hash('sha256');// or use pipelineresponse.pipe(hashStream).pipe(file);
Or use stream.pipeline(response, hashTransform, file, callback) for proper error propagation.
fs.rmdirSync(temporaryDir) (without { recursive: true }) will throw ENOTEMPTY if extraction or any intermediate step left extra files in the temp directory. That error is swallowed by the surrounding catch {}, leaking the temporary directory silently. Use fs.rmSync(temporaryDir, { recursive: true, force: true }) to guarantee cleanup.
💡 Suggested Change
Before:
if (fs.existsSync(archivePath)) fs.unlinkSync(archivePath);
fs.rmdirSync(temporaryDir);
After:
if (fs.existsSync(archivePath)) fs.unlinkSync(archivePath);
fs.rmSync(temporaryDir, { recursive: true, force: true });
43. scripts/postinstall.js (L100-L103)
makeExecutable is called without await, while all preceding steps are awaited. Based on the unchanged implementation below (which uses spawn and wraps the result in a Promise identical to clearQuarantine), makeExecutable returns a Promise. Dropping it means: (1) errors during chmod are silently ignored, (2) the finally cleanup block may run and delete the archive before makeExecutable finishes. Add await.
All three jobs (metadata, build, assemble) lack timeout-minutes, so they can run indefinitely and consume runner resources if a step hangs (e.g., a hanging npm install, long Python test run, or a stalled build). Add a reasonable timeout to each job.
Suggested values based on typical CI durations:
metadata: 20 minutes
build: 30 minutes (native compilation can be slow on macOS)
run: test -s "dist/memos-${{ needs.metadata.outputs.version }}-${{ matrix.target }}.tar.gz"
While metadata.outputs.version is validated to be a strict semver and matrix.target is a hardcoded string — so the injection risk is low in practice — the pattern is still fragile. If the value ever comes from less-trusted sources (e.g., issue title, PR body), the pattern becomes exploitable. Prefer passing values through env: blocks.
Similarly in the assemble job for --version "${{ needs.metadata.outputs.version }}".
💡 Suggested Change
Before:
- name: Synchronize version and build native archive
run: |
node scripts/sync-version.js "${{ needs.metadata.outputs.version }}"
${{ matrix.script }}
The build job checks out with fetch-depth: 0 (full history), but build only needs to build binaries — it doesn't use git history. Fetching all history on every matrix leg (4 platforms) is wasteful and slows each build. Use the default shallow clone (fetch-depth: 1) or omit fetch-depth entirely.
The assemble job and both the build job and metadata job do not use dependency caching (actions/cache or the built-in npm/pip cache options in setup-node/setup-python). Every run reinstalls all npm and Python dependencies from scratch, significantly increasing wall-clock time. Consider enabling built-in caching:
The assemble job uses ${{ needs.metadata.outputs.version }} directly inside the run: block of Validate complete asset matrix and append the manifest. While the version is validated upstream, prefer passing it through env: to be consistent with safe-shell-interpolation patterns used elsewhere in the file (e.g., the metadata job's run blocks).
The assemble job's Confirm read-only inspection contract step hard-codes the list of required files, including docs-preview.md and docs-preview.json. However, those files are only conditionally generated (when the Doc Agent draft endpoint is configured and succeeds). If the Doc Agent is not configured (secrets not set), the step will fail with a confusing "file not found" error instead of a meaningful message. The metadata job's own validation step correctly uses [ -s ... ] guards; apply the same approach here or document this dependency explicitly.
💡 Suggested Change
Before:
for file in release-notes.md evidence.json quality-report.json docs-preview.md docs-preview.json release-assets-manifest.json npm-pack.json; do
test -s "inspection/${file}"
done
After:
for file in release-notes.md evidence.json quality-report.json release-assets-manifest.json npm-pack.json; do
if ! test -s "inspection/${file}"; then
echo "::error::Required inspection file missing or empty: inspection/${file}"
exit 1
fi
done
# docs-preview files are only generated when Doc Agent is configured
for file in docs-preview.md docs-preview.json; do
if ! test -s "inspection/${file}"; then
echo "::warning::Optional inspection file missing or empty: inspection/${file}"
fi
done
The assemble job lacks a timeout-minutes setting. Since it downloads artifacts from multiple matrix legs and runs validation scripts, it could hang indefinitely.
The replacement string $1"${version}" is passed directly to String.prototype.replace. JavaScript's replace treats $ in the replacement string as a special pattern: $1, $2, $&, $$, etc., are all interpreted as references. If a version string ever contains a $ character (e.g., a pre-release identifier like 1.0.0-$special), it will be misinterpreted and silently produce a corrupt pyproject.toml. The same issue applies to the initText replacement on the next line.
Fix: Use a replacer function instead of a template-literal string so the replacement is always treated as a literal.
The idempotency guard uses String.prototype.includes() as a fallback: if the regex replacement produced no change, the code suppresses the error when the target version string appears anywhere in the file. However, version = "<ver>" may legitimately appear in other sections of pyproject.toml (e.g., a dependency pin like my-lib = "1.2.3" or a tool config block), which would cause this guard to silently accept a failed update. The check should instead verify that the version string appears specifically under the [project] section.
💡 Suggested Change
Before:
if (pyprojectText === contents.pyproject && !contents.pyproject.includes(`version = "${version}"`)) {
throw new Error("Could not update [project].version in pyproject.toml");
}
After:
const projectVersionRe = /\[project\][\s\S]*?\nversion\s*=\s*"([^"]+)"/;
const pyprojectMatch = pyprojectText.match(projectVersionRe);
if (!pyprojectMatch || pyprojectMatch[1] !== version) {
throw new Error("Could not update [project].version in pyproject.toml");
}
54. scripts/sync-version.js (L60)
When invoked via the package.json script "check-version": "node scripts/sync-version.js --check", no positional version argument is passed and RELEASE_VERSION is not set in that context, so version resolves to undefined. This causes normalizeVersion(undefined) to throw Invalid release version: (empty) — a confusing error that gives no hint that a version argument is required. A dedicated early check with a clear usage message would be far more actionable.
The per-target validation checks asset.url with .startsWith("https://"), but never verifies that asset.url actually references the expected asset file (expectedName). A contract where asset.name is correct but asset.url points to a completely different file (or a different version) would silently pass this check.
Consider also asserting that asset.url ends with or contains expectedName:
require('../release-assets.json') is called at module load time unconditionally. If release-assets.json is missing or malformed JSON (e.g., in a fresh clone before generation), the entire script will throw an unhandled MODULE_NOT_FOUND or SyntaxError crash instead of emitting a clean, user-friendly error message via issues.push().
Consider wrapping the require in a try/catch and pushing a clear error message:
let releaseAssets;
try {
releaseAssets = require("../release-assets.json");
} catch {
console.error("Prepublish checks failed:");
console.error("- release-assets.json is missing or contains invalid JSON.");
process.exit(1);
}
57. scripts/prepublish-check.js (L41-L44)
The targets comparison uses JSON.stringify([...releaseAssets.targets].sort()) which treats the arrays as strings of JSON. While functional for simple string arrays, this approach is fragile: it is sensitive to how JSON.stringify serializes values (e.g., objects, numbers, or null entries in the array would silently produce incorrect results). A more robust and readable approach is to use explicit set comparison or every()/length checks:
💡 Suggested Change
Before:
if (
!Array.isArray(releaseAssets.targets) ||
JSON.stringify([...releaseAssets.targets].sort()) !== JSON.stringify([...expectedTargets].sort())
) {
The file is read twice per asset — once for SHA-256 and once for MD5. Beyond the wasted I/O for potentially large binary release assets, a file modified between the two reads (e.g., by a concurrent build step) would produce a SHA-256 and MD5 pair that don't correspond to the same content. This inconsistent integrity record is then used downstream for OSS upload verification.
Read the bytes once and feed them to both hash functions:
data = path.read_bytes()
"sha256": hashlib.sha256(data).hexdigest(),
"md5": hashlib.md5(data, usedforsecurity=False).hexdigest(),
59. scripts/upload_release_assets.py (L212-L217)
The broad except Exception catches the deliberate RuntimeError("OSS object {key} exists with different content; refusing overwrite") raised just above (line 202). A deterministic guard that's intentionally non-retryable gets silently retried three times, then wrapped in a misleading multi-attempt error message. The same applies to authentication errors or bucket-not-found errors from the OSS SDK — retrying them wastes time and produces a confusing error trail.
Re-raise immediately for errors that are known to be non-retryable:
💡 Suggested Change
Before:
except Exception as exc:
errors.append(f"attempt {attempt}: {type(exc).__name__}: {exc}")
if attempt == 3:
report_exhausted_failure(version, key, errors)
raise RuntimeError(f"OSS operation failed after three attempts for {key}: {'; '.join(errors)}") from exc
time.sleep(attempt)
After:
except RuntimeError:
raise
except Exception as exc:
errors.append(f"attempt {attempt}: {type(exc).__name__}: {exc}")
if attempt == 3:
report_exhausted_failure(version, key, errors)
raise RuntimeError(f"OSS operation failed after three attempts for {key}: {'; '.join(errors)}") from exc
time.sleep(attempt)
60. scripts/upload_release_assets.py (L200-L201)
On Aliyun OSS, objects uploaded via multipart upload have an ETag of the form MD5(part1_md5 + part2_md5 + ...)-N, not a plain MD5 of the full file content. put_object_from_file switches to multipart automatically for large files. When the same asset is re-uploaded in a subsequent run, remote_etag will not equal asset["md5"] (the local file MD5), and the idempotency check will raise "OSS object exists with different content; refusing overwrite" even though the file is identical. This makes the entire idempotency guarantee unreliable for any asset large enough to trigger multipart upload.
Use content-length alone as the idempotency signal, or store and compare against a custom x-oss-meta-sha256 object metadata tag set at upload time:
💡 Suggested Change
Before:
remote_etag = str(remote.etag or "").strip('"').lower()
if int(remote.content_length) != int(asset["size"]) or remote_etag != asset["md5"]:
After:
# ETag for multipart uploads is not a plain MD5; compare size only,
# and rely on the sha256 stored in the runtime contract for integrity.
if int(remote.content_length) != int(asset["size"]):
61. scripts/upload_release_assets.py (L207-L209)
The same multipart-ETag mismatch applies to the post-upload verification check (line 208). After a fresh put_object_from_file that used multipart upload, the OSS ETag will not equal the local file MD5, so this check will always raise "OSS verification failed for {key}", causing every large-file upload to appear to have failed even when it succeeded correctly.
Consistently use size-only comparison here too, or read back a custom metadata header:
💡 Suggested Change
Before:
remote_etag = str(remote.etag or "").strip('"').lower()
if int(remote.content_length) != int(asset["size"]) or remote_etag != asset["md5"]:
raise RuntimeError(f"OSS verification failed for {key}")
After:
# ETag is not a plain MD5 for multipart uploads; size is the reliable check.
if int(remote.content_length) != int(asset["size"]):
raise RuntimeError(f"OSS verification failed for {key}")
62. tests/test_release_assets.py (L13-L22)
expected_assets derives the target list by calling load_contract(), which reads release-assets.json at runtime. The test hard-codes four specific filenames, duplicating the contract file. If a platform is added or removed from release-assets.json, this assertion silently becomes stale and never catches the mismatch.
Consider reading release-assets.json in the test (or mocking load_contract) and constructing the expected list dynamically:
This keeps the test honest without hard-coding the platform matrix.
63. tests/test_release_assets.py (L76-L86)
Every test for validate_live_contract exercises an error branch; there is no test for the valid/happy path. A regression that makes validate_live_contract always raise (e.g., a mistakenly tightened guard) would be invisible to this suite, since all tests already expect a RuntimeError.
Add at least one test that passes a fully valid production-like config and asserts the function returns without raising:
deftest_live_contract_accepts_valid_production_config(self) ->None:
# Should not raisevalidate_live_contract(
{
"bucket": "memos-release",
"endpoint": "https://oss-cn-shanghai.aliyuncs.com",
"region": "cn-shanghai",
"public_base_url": "https://downloads.example.com/memos-cloud-cli",
"targets": ["linux-x64"],
}
)
🧹 Filtered 4 low-confidence OCR finding(s) before posting/fix-loop (duplicate: 4).
Generated by cloud-assistant via Open Code Review.
The test environment encountered an issue that requires manual attention.
Details: Executor error: Command failed: git clone --depth 1 --branch docs-sync/memos-cli-release-notes git@github.com:MemTensor/MemOS-Cloud-CLI.git /data/test-workspaces/5fb5d93ef3f66fa4/repo
Cloning into '/data/test-workspaces/5fb5d93ef3f66fa4/repo'...
warning: Could not find remote branch docs-sync/memos-cli-release-notes to clone.
fatal: Remote branch docs-sync/memos-cli-release-notes not found in upstream origin Branch:docs-sync/memos-cli-release-notes
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
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.
Memtensor-AI
added
area:docs
Summary
contents: readwith no persisted Git credentials, npm token, OSS credentials, or publish commandsgitHead, immutable tags, GitHub Release inventory, and version PR stateValidation
npm pack --dry-run --jsonwith an isolated cacheactionlintandgit diff --checkevidence.jsonandquality-report.jsonwhile failing closedSafety / migration
v1.0.6at npmgitHeadc18ced54beeb817f6d3f0def1d43eca66da94817, or keep the migration-only baseline variable until that is done