feat: distribute the Open Flow command release - #335
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe CLI adds the Sequence Diagram(s)sequenceDiagram
participant CLI as CLI bootstrap
participant Flow as runOpenFlowCommand
participant Artifact as Open Flow artifact installer
participant Session as Cloud session
participant Gateway as Cloud gateway
CLI->>Flow: parse and delegate flow arguments
Flow->>Artifact: resolve local or bundled artifact
Artifact-->>Flow: return validated entry.js directory
Flow->>Session: resolve account and team identity
Session-->>Flow: return authorization and team headers
Flow->>Gateway: send restricted request or upload
Gateway-->>Flow: return Cloud response
Flow-->>CLI: return validated exit code
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/adapters/completion/static-completion-renderer.test.ts (1)
62-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
onlineDevEndpointinstead of hardcoding"oomol.dev".
createCliCatalogdecides visibility withendpoint !== onlineDevEndpoint(src/application/commands/catalog.tsline 54). This test hardcodes the literal"oomol.dev". If the constant changes, the production catalog and this test drift apart.Import the constant so the test tracks the single definition.
♻️ Proposed change
- const devOutput = renderer.render("fish", createCliCatalog("oomol.dev")); + const devOutput = renderer.render("fish", createCliCatalog(onlineDevEndpoint));Add the import alongside the existing catalog import, using the module that defines
onlineDevEndpoint.As per coding guidelines: "Never duplicate constant values across files. Define once, import or re-export with aliases elsewhere."
🤖 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/adapters/completion/static-completion-renderer.test.ts` around lines 62 - 70, Update the test around StaticCompletionRenderer to import onlineDevEndpoint from the module that defines it and pass that constant to createCliCatalog instead of the hardcoded "oomol.dev" value, keeping the visibility assertions unchanged.Source: Coding guidelines
src/application/commands/flow-artifact.test.ts (2)
119-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for tar path traversal rejection.
The suite covers the typeflag check with a link entry. It does not cover
isNormalizedArtifactPath, which is the control that stops an archive entry from writing outside the extraction directory.Add cases that assert rejection for an entry path containing a
..segment and for an absolute path. Both go through the sameencodeTarGziphelper already in this file.💚 Suggested additional cases
+ test("rejects tar entries that escape the artifact root", async () => { + const archive = encodeTarGzip([{ + body: new Uint8Array(), + mode: 0o644, + path: "open-flow-command/../escape.js", + type: "0", + }]); + const release = createRelease(archive); + const environment = await createTestEnvironment(); + + await expect(installOpenFlowCommandRelease(release, { + env: environment.env, + execPath: process.execPath, + fetcher: createArchiveFetcher(archive), + })).rejects.toThrow("invalid file path"); + });🤖 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/application/commands/flow-artifact.test.ts` around lines 119 - 134, Add test coverage in the flow artifact installation tests for isNormalizedArtifactPath by creating archives with one entry using a path containing a ".." segment and another using an absolute path. Reuse encodeTarGzip and the existing installOpenFlowCommandRelease assertion pattern, verifying both cases reject before extraction.
136-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe concurrency test passes even when no serialization occurs.
The test resolves
resumeRequestimmediately after creatingsecond. Nothing guarantees thatsecondreachesacquireDownloadTempLockbeforefirstcompletes its install. Iffirstfinishes first,secondtakes the cache-hit return atflow-artifact.tsline 133 and never contends the lock.requestCountis still1and both assertions still pass.The test therefore cannot distinguish lock serialization from plain cache reuse.
To make the assertion meaningful, hold the first request open until
secondhas demonstrably started, then release:💚 Suggested restructure
const first = installOpenFlowCommandRelease(fixture.release, options); await requestStarted.promise; const second = installOpenFlowCommandRelease(fixture.release, options); - resumeRequest.resolve(); + // Give `second` time to reach the lock while the first download is still blocked. + await Bun.sleep(50); + expect(requestCount).toBe(1); + resumeRequest.resolve();This proves the second call did not issue its own request while the first held the lock.
🤖 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/application/commands/flow-artifact.test.ts` around lines 136 - 166, Restructure the test “serializes concurrent installation of the same digest” so the first fetch remains blocked until the second installation has demonstrably started and attempted to contend with the download lock. Add an explicit synchronization signal for the second call’s fetch/lock-entry path, await that signal before resolving resumeRequest, then retain the assertions that both calls share the directory and requestCount is one.src/application/commands/flow-artifact.ts (3)
133-135: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvery
oo flowinvocation re-hashes the whole cached artifact.Line 133 calls
validCommandArtifactDirectorybefore any lock is taken, on the normal cache-hit path. That runsvalidateCommandArtifactDirectory, which walks the directory tree and then reads and SHA-256 hashes every manifest-listed file (lines 651-654). The pinned release archive is about 5 MB, so eachoo flowinvocation pays a full read and hash of the expanded artifact before Open Flow starts.The cache directory is already named by the archive digest and is written through an atomic
renamefrom a private extraction directory, so a correct cache entry cannot be partially written. The full re-hash defends only against post-install local tampering or disk corruption.Consider a cheaper steady-state check and keep the full verification for the repair path. Options:
- Compare the file set plus each file's size and mtime against a stamp written at install time, and fall back to full hashing only on mismatch.
- Write a
.verifiedmarker containing the digest after a successful install, and validate fully only when the marker is absent.♻️ Sketch: fast path first, full validation as fallback
- if (await validCommandArtifactDirectory(commandDirectory, release)) { + if (await cachedArtifactLooksIntact(commandDirectory, release)) { return commandDirectory; }Add a helper that compares the recorded file set, sizes, and mtimes, and only calls
validateCommandArtifactDirectorywhen that comparison fails.Also applies to: 629-655
🤖 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/application/commands/flow-artifact.ts` around lines 133 - 135, Update the cache-hit logic around validCommandArtifactDirectory so normal oo flow invocations use a cheap integrity check first, comparing the recorded artifact file set, sizes, and mtimes from installation; invoke validateCommandArtifactDirectory only when that check fails or no install marker exists. Preserve the existing full validation and repair behavior for mismatches, and apply the same fast-path handling to the related validation flow near the artifact install logic.
219-291: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout or idle-progress guard to
downloadCommandArchive.
FetcherandcreateRetryingFetcherdo not impose a deadline. A stalled response can blockoo flowindefinitely while holding the download lock. Pass anAbortSignaldeadline or add an idle-byte watchdog, consistent with the self-update download path.🤖 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/application/commands/flow-artifact.ts` around lines 219 - 291, Update downloadCommandArchive to enforce a download deadline or idle-progress timeout, using the same timeout/watchdog approach as the self-update download path. Apply it to the fetch request and ensure stalled responses abort and release the reader and file handle instead of blocking indefinitely.
363-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid coupling archive acceptance to local compressor output.
downloadCommandArchivechecks the complete response length and SHA-256 digest beforedecodeCommandArchiveruns. The fullgzipSync(tar, { level: 9 })comparison adds a compressor-output constraint, not archive integrity. Differentnode:zlibimplementations or versions can reject the pinned archive with"not canonically encoded". Replace it with an implementation-independent check for trailing or truncated gzip data if that validation remains required.🤖 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/application/commands/flow-artifact.ts` around lines 363 - 380, Update decodeCommandArchive and canonicalGzip so archive acceptance no longer depends on local gzipSync output; retain the existing full-length and SHA-256 validation in downloadCommandArchive, and replace the compressor comparison with an implementation-independent check that rejects truncated or trailing gzip data if needed.src/application/bootstrap/run-cli.ts (1)
338-344: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReport the real delegated argument count.
argCountandflagsCountare always0, even whenopenFlowInvocation.argsis not empty. Theflowtelemetry event therefore reports every invocation as argument-free, and the dimension carries no signal. An argument count is a count, not free-form input, so it stays privacy safe. Prefer a bucket helper fromsrc/application/telemetry/buckets.tsif one exists for counts.♻️ Proposed change
telemetryRecorder.observer.onCommandResolved?.({ - argCount: 0, + argCount: openFlowInvocation.args.length, commandPath: ["flow"], excludeFromTelemetry: false, - flagsCount: 0, + flagsCount: openFlowInvocation.args.filter( + argument => argument.startsWith("-"), + ).length, outputFormat: "text", });As per coding guidelines: "For useful command-specific dimensions, call
context.telemetry?.recordProperties(...)from the command handler with only low-cardinality, privacy-safe enums, booleans, counts, or buckets."🤖 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/application/bootstrap/run-cli.ts` around lines 338 - 344, Update the flow telemetry payload in the command-resolution path to report the real delegated argument count from openFlowInvocation.args instead of hardcoding argCount to zero, and derive flagsCount from the delegated flags when available. Reuse the existing count-bucketing helper from buckets.ts if applicable, while preserving the privacy-safe, low-cardinality telemetry contract.Source: Coding guidelines
🤖 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 `@src/application/bootstrap/run-cli.ts`:
- Around line 337-353: Verify the intended endpoint scope for the open-flow path
in the run-cli flow invocation branch. If flow is restricted to oomol.dev, guard
the runOpenFlowCommand call and its related telemetry with the same OO_ENDPOINT
check used by createCliCatalog; otherwise preserve execution on all endpoints
and ensure the catalog visibility behavior is intentional.
In `@src/application/commands/flow-artifact.ts`:
- Around line 732-750: Update the purge handling in the uninstall command to
remove both storePaths.dataDirectory and the Open Flow artifact cache resolved
by resolveCommandCacheRoot. Ensure --purge deletes the platform-specific cache
root in addition to the existing local SQLite data cache, while leaving
non-purge uninstall behavior unchanged.
---
Nitpick comments:
In `@src/adapters/completion/static-completion-renderer.test.ts`:
- Around line 62-70: Update the test around StaticCompletionRenderer to import
onlineDevEndpoint from the module that defines it and pass that constant to
createCliCatalog instead of the hardcoded "oomol.dev" value, keeping the
visibility assertions unchanged.
In `@src/application/bootstrap/run-cli.ts`:
- Around line 338-344: Update the flow telemetry payload in the
command-resolution path to report the real delegated argument count from
openFlowInvocation.args instead of hardcoding argCount to zero, and derive
flagsCount from the delegated flags when available. Reuse the existing
count-bucketing helper from buckets.ts if applicable, while preserving the
privacy-safe, low-cardinality telemetry contract.
In `@src/application/commands/flow-artifact.test.ts`:
- Around line 119-134: Add test coverage in the flow artifact installation tests
for isNormalizedArtifactPath by creating archives with one entry using a path
containing a ".." segment and another using an absolute path. Reuse
encodeTarGzip and the existing installOpenFlowCommandRelease assertion pattern,
verifying both cases reject before extraction.
- Around line 136-166: Restructure the test “serializes concurrent installation
of the same digest” so the first fetch remains blocked until the second
installation has demonstrably started and attempted to contend with the download
lock. Add an explicit synchronization signal for the second call’s
fetch/lock-entry path, await that signal before resolving resumeRequest, then
retain the assertions that both calls share the directory and requestCount is
one.
In `@src/application/commands/flow-artifact.ts`:
- Around line 133-135: Update the cache-hit logic around
validCommandArtifactDirectory so normal oo flow invocations use a cheap
integrity check first, comparing the recorded artifact file set, sizes, and
mtimes from installation; invoke validateCommandArtifactDirectory only when that
check fails or no install marker exists. Preserve the existing full validation
and repair behavior for mismatches, and apply the same fast-path handling to the
related validation flow near the artifact install logic.
- Around line 219-291: Update downloadCommandArchive to enforce a download
deadline or idle-progress timeout, using the same timeout/watchdog approach as
the self-update download path. Apply it to the fetch request and ensure stalled
responses abort and release the reader and file handle instead of blocking
indefinitely.
- Around line 363-380: Update decodeCommandArchive and canonicalGzip so archive
acceptance no longer depends on local gzipSync output; retain the existing
full-length and SHA-256 validation in downloadCommandArchive, and replace the
compressor comparison with an implementation-independent check that rejects
truncated or trailing gzip data if needed.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abd90123-9b87-40dd-a205-928ac3e0966a
📒 Files selected for processing (19)
docs/commands.mddocs/commands.zh-CN.mdsrc/adapters/completion/static-completion-renderer.test.tssrc/application/bootstrap/run-cli.tssrc/application/commands/catalog.tssrc/application/commands/connector/shared.tssrc/application/commands/file/download.tssrc/application/commands/file/download/file-system.test.tssrc/application/commands/file/download/file-system.tssrc/application/commands/flow-artifact.test.tssrc/application/commands/flow-artifact.tssrc/application/commands/flow-release.tssrc/application/commands/flow.cli.test.tssrc/application/commands/flow.tssrc/application/commands/shared/download-progress.test.tssrc/application/commands/shared/download-progress.tssrc/application/commands/team/identity.tssrc/application/commands/telemetry-decisions.test.tssrc/i18n/catalog.ts
|
Review follow-up for the non-threaded suggestions I am not adopting:
The actionable archive timeout, tar-path, concurrency, gzip compatibility, and uninstall purge feedback is addressed in commit 8e45c65. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/application/self-update/uninstall.test.ts (1)
203-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
resolveOpenFlowCommandCacheRootfor the expected path.This test duplicates the platform and cache-format rules from
resolveOpenFlowCommandCacheRoot. Build the expected value with that resolver and the same environment, home directory, and platform used to build the uninstall plan.Proposed fix
- expect(paths(userData)).toContain(join( - tempHome, - ".cache", - "oo", - "open-flow", - "command-artifact-v1", - )); + expect(paths(userData)).toContain( + resolveOpenFlowCommandCacheRoot({ + env: { HOME: tempHome }, + homeDirectory: tempHome, + platform: "linux", + }), + );As per coding guidelines, replace test expressions that duplicate extracted production logic with the shared utility.
🤖 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/application/self-update/uninstall.test.ts` around lines 203 - 209, Update the expected cache path assertion in the uninstall test to use resolveOpenFlowCommandCacheRoot with the same environment, home directory, and platform passed when constructing the uninstall plan, instead of manually joining cache path segments. Keep the assertion verifying that paths(userData) contains the resolver’s returned value.Source: Coding guidelines
🤖 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 `@src/application/commands/flow-artifact.test.ts`:
- Around line 158-173: Update the “guards archive downloads with a timeout
signal” test to use fake timers and make its createArchiveFetcher callback
reject when the captured signal emits abort. Advance the timer by the configured
300,000 ms timeout, assert installOpenFlowCommandRelease rejects, and verify the
installation cleanup removes temporary files.
---
Nitpick comments:
In `@src/application/self-update/uninstall.test.ts`:
- Around line 203-209: Update the expected cache path assertion in the uninstall
test to use resolveOpenFlowCommandCacheRoot with the same environment, home
directory, and platform passed when constructing the uninstall plan, instead of
manually joining cache path segments. Keep the assertion verifying that
paths(userData) contains the resolver’s returned value.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7655e718-c33b-4261-9436-66086c3ca260
📒 Files selected for processing (5)
src/application/commands/flow-artifact.test.tssrc/application/commands/flow-artifact.tssrc/application/commands/uninstall.cli.test.tssrc/application/self-update/uninstall.test.tssrc/application/self-update/uninstall.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/application/commands/flow-artifact.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/application/commands/flow-artifact.ts (1)
266-305: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the response body reader on the failure path.
The inner
finallycallsreader.releaseLock()but never cancels the stream. Ifinvalid(...)throws at Line 275 or Line 289, the remaining body is left unread and the connection is held until the abort timer fires atcommandArchiveDownloadTimeoutMs. Cancel the reader so the socket is released immediately.♻️ Proposed change to cancel the reader before releasing the lock
finally { - reader.releaseLock(); + await reader.cancel().catch(() => {}); + reader.releaseLock(); await fileHandle.close(); }🤖 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/application/commands/flow-artifact.ts` around lines 266 - 305, Update the download cleanup in the inner finally block around reader and fileHandle so the response body reader is cancelled before releaseLock(), ensuring failures from invalid(...) terminate the remaining stream immediately while preserving the existing file close behavior.
🤖 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.
Nitpick comments:
In `@src/application/commands/flow-artifact.ts`:
- Around line 266-305: Update the download cleanup in the inner finally block
around reader and fileHandle so the response body reader is cancelled before
releaseLock(), ensuring failures from invalid(...) terminate the remaining
stream immediately while preserving the existing file close behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b851dbe-8ad2-4ec9-84d5-48b6658ea284
📒 Files selected for processing (1)
src/application/commands/flow-artifact.ts
1 participant
No description provided.