fix github actions build#41
Conversation
meanmail
left a comment
There was a problem hiding this comment.
Review of the framework-lesson storage rework.
The change looks well thought out and is generously commented, and the RemoveTaskFile.apply fix (state -= path instead of state[path] = text) is a real, correct bug fix. A few cross-cutting points in addition to the inline comments:
- Mixed line endings.
FrameworkLessonManagerImpl.ktnow mixes CRLF and LF (base had ~1689 CRLF lines, this branch ~1479), so many-/+pairs in the diff are byte-for-byte identical and only differ by EOL. This roughly doubles the diff and hides the real changes. Suggest adding a.gitattributes(*.kt text eol=lf) and normalizing EOLs in a separate, content-free commit. LOG.warnfor debug output. SeveralLOG.warn("saveExternalChanges: ... keys=...")/ChangeFile.apply: ...messages are debug-level detail logged at WARN. This will be noisy in production logs; please downgrade todebug/trace.
LegacyFrameworkStorage:
- import java.io.ByteArrayInputStream instead of inline fully-qualified names
- document the deliberate version fall-through in both record readers
Logging:
- downgrade debug-level LOG.warn traces to LOG.debug in FrameworkLessonManagerImpl
and UserChanges; keep genuine warnings/caught exceptions at WARN
Cleanup:
- drop the SlowOperations.knownIssue("EDU-XXXX") placeholder wrappers and the
now-unused import (call YamlFormatSynchronizer.saveItem directly)
Remove isUnitTestMode test-only forks (use injectable seams so tests exercise
the real production path):
- FrameworkLessonManagerImpl: remove the test-only currentTask.record assignment;
the update test now seeds the legacy record itself
- HyperskillOpenInIdeRequestHandler: inject the stages executor (and loader) so
the CompletableFuture/timeout/cancellation path runs in tests instead of a
synchronous short-circuit; add HyperskillLoadStagesWithTimeoutTest covering
Ok / timeout / failure / ProcessCanceledException
Add a `*.kt text eol=lf` rule to .gitattributes (the repo previously disabled normalization globally via `* -crlf`) and renormalize the two .kt files that still had CRLF/mixed endings: - FrameworkLessonManagerImpl.kt (was mixed CRLF/LF, which doubled its PR diff) - FrameworkLessonUserFilesPropagationTest.kt This is a content-free change (line endings only); no source was modified.
meanmail
left a comment
There was a problem hiding this comment.
Re-review — approved ✅
Thanks for the thorough follow-up. I re-checked the changes against the new revision (49d0371) rather than just the commit messages, and the previous round of feedback is handled well.
Addressed (verified in code):
- Removed the test-only
currentTask.recordfork inFrameworkLessonManagerImpl; the update test now seeds the legacy record itself. - Dropped the
EDU-XXXXplaceholder wrappers —YamlFormatSynchronizer.saveItemis called directly. HyperskillOpenInIdeRequestHandlernow injects the stages executor/loader, so theCompletableFuture/ timeout / cancellation path runs under tests (HyperskillLoadStagesWithTimeoutTestcovers Ok / timeout / failure /ProcessCanceledException).LegacyFrameworkStorage:java.io.ByteArrayInputStreamimported; the deliberate version fall-through documented in both readers.- Debug-level
LOG.warntraces downgraded toLOG.debug; remaining WARN logs are genuine failures/caught exceptions. .ktline endings normalized to LF in a dedicated commit — the diff is reviewable now.pluginVerification.failureLevel: accepted the rationale (keep the marketplace-validated build green).
The RemoveTaskFile.apply fix (state -= path) remains the core correctness win.
Non-blocking, left open for your call:
PropagationConflictDialog.kt:198still has anisUnitTestModebranch — same injectable-seam idea as the Hyperskill handler would let tests exercise the realresultpath. A test-quality nit, not a correctness issue.RemoteEduTaskYamlMixin.checkProfileswitchedALWAYS→NON_EMPTY: just confirm no backend/round-trip consumer requirescheck_profileto always be present.
Neither is a blocker, so approving. Could you resolve the addressed threads on your side? I don't have permission to resolve them in this repo.
This branch overhauls how framework lessons persist and propagate learner changes across stages, replacing the legacy diff-based storage with a git-like content-addressable history (snapshots / commits / refs), and fixes the resulting navigation, course-update, and submission flows. It also gets CI and the :intellij-plugin:test suite back to green.
Why
The legacy diff-based framework storage lost or mis-propagated learner code across stages (ALT-10961, ALT-10998, ALT-10961 follow-ups). A self-contained snapshot history makes navigation/update deterministic and restart-safe, and fixes the data-loss bugs the failing tests captured.
What changed
FrameworkLessonManagerImpl reworked to store full per-stage snapshots as content-addressable blobs with commits and stage_ / step_ refs and a HEAD (see framework/impl/FRAMEWORK_STORAGE.md). Full contents (not diffs) are stored so state survives IDE restart / offline.
FrameworkStorage / LegacyFrameworkStorage / framework/storage/UserChanges updated; legacy records are migrated on the fly.
Navigation propagation reworked: snapshot-based target state, ancestor checks to decide when a merge is needed, and a Keep / Replace conflict path (PropagationConflictDialog) with auto-Keep/auto-Replace heuristics based on whether each side has learner changes.
FrameworkTaskUpdateInfo, FrameworkLessonHistory, UpdateUtils: when a course update adds/removes/changes a framework task file, the learner's local edits are preserved instead of being overwritten; author content is kept in the task model for later revert.
SolutionLoaderBase, TaskNavigationAction, handlers/handlersUtils, HyperskillUtils, HyperskillOpenInIdeRequestHandler, submissions/utils, ext/TaskExt/StudyItemExt, VirtualFileExt adjusted for the new storage model (original test/template file caching, ref handling, rename/move).