fix github actions build#41
Conversation
meanmail
left a comment
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.
| currentState: Map<String, String>, | ||
| targetState: Map<String, String> | ||
| ): Result { | ||
| if (ApplicationManager.getApplication().isUnitTestMode) { |
There was a problem hiding this comment.
Test-mode branch substitutes a different dialog implementation, so the real PropagationConflictDialog.result path is never exercised by tests. Acceptable for headless runs, but worth abstracting the conflict decision behind an interface that can be replaced in tests.
There was a problem hiding this comment.
This one still looks unaddressed in the latest revision (isUnitTestMode branch at PropagationConflictDialog.kt:198). Same idea as the Hyperskill handler fix — could the conflict decision be injected so tests exercise the real result path instead of a test-only fork? Leaving open.
There was a problem hiding this comment.
The test-mode branch uses Messages.showYesNoDialog, which routes through IntelliJ's TestDialogManager. Tests can already drive the decision deterministically via our withTestDialog(TestDialog.YES/NO) helper, so the conflict choice is replaceable in tests without a dedicated interface. The custom DialogWrapper UI can't be rendered headless regardless, so a separate abstraction wouldn't add testable surface. Keeping the current idiomatic approach.
| @get:JsonProperty(CHECK_PROFILE) | ||
| @set:JsonProperty(CHECK_PROFILE) | ||
| @get:JsonInclude(JsonInclude.Include.ALWAYS) | ||
| @get:JsonInclude(JsonInclude.Include.NON_EMPTY) |
There was a problem hiding this comment.
ALWAYS -> NON_EMPTY: an empty checkProfile will now be omitted from serialization. Please confirm the backend / YAML round-trip tolerates a missing check_profile field. Note EduTaskReply.score is changed in the opposite direction (to ALWAYS) in this same PR.
There was a problem hiding this comment.
Gentle follow-up: did you confirm the backend / YAML round-trip is fine with check_profile omitted when empty (no consumer expects the field to always be present)? Leaving open until confirmed.
There was a problem hiding this comment.
Confirmed safe. A missing check_profile deserializes to the "" default (RemoteEduTask.checkProfile), and StudentTaskChangeApplier guards with isNotEmpty() before applying, so an omitted field is a no-op. The existing test remote edu task serialization test asserts no check_profile line for a profile-less task, which only holds under NON_EMPTY. This is local YAML round-trip only — the backend submission payload uses the separate hyperskillAPI/StepikAPI mixins. EduTaskReply.score → ALWAYS is the opposite direction intentionally because score is required in the submission payload.
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
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.
2 participants
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).