feat(helioslite): safely import verified Forge session snapshots - #147
Conversation
Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Co-Authored-By: ForgeCode <noreply@forgecode.dev>
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2371e4038d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "context", | ||
| "context_zstd", | ||
| "is_compressed", | ||
| "hidden", |
There was a problem hiding this comment.
The current Forge conversations schema in crates/forge_repo/src/database/schema.rs and the migrations have no hidden column, but export_forge_snapshot requires it during validate_schema before reading any rows. As a result, importing a normal Forge database created by this repo fails with missing columns: hidden, making helioslite sessions import-forge unusable unless the DB came from the test-only fixture. Drop this requirement/query or add a migration before gating imports on it.
Useful? React with 👍 / 👎.
| if destination.exists() { | ||
| return Err(anyhow::anyhow!( | ||
| "snapshot destination already exists: {}", | ||
| destination.display() | ||
| )); |
There was a problem hiding this comment.
This preflight rejects every existing destination, but publish_snapshot_atomic is explicitly written to verify a matching existing bundle and no-op. When a user reruns helioslite sessions import-forge --dest <same dir> after a successful import, validation fails here before provenance/content checks run, so the idempotent import path advertised by the snapshot publisher is unreachable.
Useful? React with 👍 / 👎.
| if candidate == forge_root | ||
| || candidate.starts_with(&forge_root) | ||
| || forge_root.starts_with(candidate) |
There was a problem hiding this comment.
The overlap guard compares candidate exactly as provided against an absolute home.join(".forge"). If HeliosLite is launched from $HOME with HELIOSLITE_HOME=.forge (or through a symlink to ~/.forge), this check does not fire and HeliosLite uses the standard Forge root for its database, credentials, and other runtime files, defeating the isolation this change is trying to enforce. Resolve relative paths and symlinks before comparing with the Forge root.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
WARNING: File is 760 lines, exceeding the 500-line hard limit
The review policy requires files to be ≤500 lines. This new module should be decomposed into smaller, focused units (e.g., separate schema validation, publishing, and fingerprinting modules).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 New Issue + 3 Existing Issues | Recommendation: Address before merge (PR already merged) Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 112.6K · Output: 26.4K · Cached: 1.5M |
…shot import) Resolved forge_config/reader.rs conflict by keeping the remote binary-aware resolution (HELIOSLITE_HOME with overlap validation, config_dir/sessions_path) while restoring Gate 5 legacy ~/.forge precedence for the helioslite binary and the public binary_prefix() used by the updater and heliosdoctor. cli.rs/ui.rs auto-merged; workspace check and forge_config tests (52) pass.
Summary
Integrates the four reviewed HeliosLite session-import commits on current
main:sessions import-forgeCLI dispatch and containment checks;Safety invariants
-wal,-shm,-journal) fail closed.~/.forgeare rejected.Local evidence
cargo test -q -p forge_repo conversation::snapshot::tests-> 4 passedcargo test -q -p forge_main --lib ui::tests-> 6 passedgit diff --check-> passedNo package, tag, release, or merge is included in this PR.