[RAPTOR-19525] feat(workload): add --verify flag and surface skipped symlinks - #868
Open
ajalon1 wants to merge 37 commits into
Open
[RAPTOR-19525] feat(workload): add --verify flag and surface skipped symlinks#868ajalon1 wants to merge 37 commits into
ajalon1 wants to merge 37 commits into
Conversation
|
🎫 Jira: |
ajalon1
marked this pull request as ready for review
August 29, 2026 05:33
…stent server model Replace the shallow fakeFilesClient stub with an in-memory server model that records per-path content and SHA-256, models stage REPLACE merge semantics, supports configurable AllFiles pagination, returns numFiles counting all files in the resulting version, and exposes per-method call counters and fault-injection hooks. Guard every shared map with a mutex so -race is clean under 4-way concurrent uploads. Add a syncedProject helper that builds a temp project tree with initialized workload state (config.json + manifest.json matching file hashes) for reuse by later features. Add contract tests for VAL-UPLOAD-015 (empty plan issues zero upload-side calls) and VAL-UPLOAD-017 (dry-run issues zero upload-side calls), with a positive control proving the counters have teeth. Zero production (non-_test.go) files modified. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ifest records what was sent
The manifest recorded the Phase-2 planned hash for uploaded files, not the
hash of the bytes actually sent. A file rewritten between plan and upload left
BASE describing content the server never received, and the next sync then
reported "Up to date." forever — the TOCTOU poisoning reproduced 3/3 through
the real binary.
ApplyUploads now returns UploadOutcome{CatalogID, VersionID, Sent} where Sent
carries the per-path SHA-256 and size of the bytes that crossed the wire. The
stage path wraps the open file in io.TeeReader(f, sha256.New()) and derives
content-length from f.Stat() on the already-open handle (never a fresh
os.Stat). The zip path wraps the archive entry writer in io.MultiWriter(w,
sha256.New()) and takes the streamed size from io.Copy's return. The shared
hash-while-streaming piece is factored into hashstream.go so jscpd stays quiet.
Phase 6 seeds each uploaded path's manifest entry from outcome.Sent[path] and
hard-fails with an error naming the path if an entry is missing — a per-path
fallback to the Phase-2 hash is the original bug and must not exist anywhere.
The Engine struct gains an uploadOutcome field as the channel between Phase 5
and Phase 6 (runPhases has no per-phase return threading). Manifest schema stays
at version 1; no new fields are added to manifest.json or config.json.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Add internal/workload/sync/upload_zip_test.go, the first test file for the zip uploader, covering the archive path's hash-while-streaming behaviour introduced by upload-outcome-streamed-hashes. Tests: - TestZipBuild_HashesStreamedBytes: a normal multi-file archive whose per-path Sent hashes equal the SHA-256 of the file contents, with a genuine read-back (open the produced archive, extract entries, hash the extracted bytes and confirm they match the recorded Sent hash). - TestZipBuild_SameSizeContentChange: a file rewritten to different bytes of the SAME size between plan and addToZip, asserting the Sent hash is the archived bytes' hash and not the Phase-2 planned hash, with read-back confirming the archive contains the build-time bytes. - TestZipBuild_SizeChange: grow and shrink cases, asserting Sent size equals the byte count that entered the archive (io.Copy's return) rather than fa.LocalSize, with hash and size self-consistent and read-back confirming the extracted bytes match. - TestStageAndZipProduceIdenticalSent: given the same project tree, the stage path and the zip path produce identical Sent maps (same hashes and sizes for every file), so Phase 6 would write identical manifests. Read-back approach: call buildZip (the production function), open the archive it returns the path to, and hash the extracted entries. This asserts "the archive contains what we claimed" rather than "we hashed something." Every test was proven to fail first by temporarily reverting addToZip's hash-while-streaming (all hash assertions failed) and buildZip's streamed-size usage (size assertions failed with the planned size instead of the streamed size). Both reverts were restored; no production code was modified. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ance persisted state Add regression coverage proving that a failed or partial upload never advances manifest.json or config.json, and that the Phase 6 hard-fail on a missing Sent entry actually holds. Eleven new test cases across four files: upload_failure_test.go (VAL-UPLOAD-011, VAL-UPLOAD-013, VAL-UPLOAD-021): - Missing Sent entry: buildNewBaseManifest hard-fails naming the path; phase6State does not write manifest.json. Also documents the SaveConfig-before-SaveManifest ordering hazard (config IS advanced when buildNewBaseManifest fails — not reachable through normal operation but a real defect, reported in handoff). - Partial Sent from worker error: uploadFilesParallel returns an error, Phase 6 never runs, state untouched. - One file fails late, all files fail, ApplyStage fails: each leaves manifest.json byte-identical, config.json unchanged, working tree rolled back, ApplyStage not called (or called but not persisted). - File deleted between Plan and Execute: error names the path, no panic, manifest/config unchanged, remaining files not applied. upload_concurrency_test.go (VAL-UPLOAD-012): - 4-way concurrent upload of 13 mixed-size files (0-byte, small, chunk-boundary 32 KiB) under -race: no race, exactly one Sent entry per file, no duplicate upload, all hashes match content. dry_run_integrity_test.go (VAL-UPLOAD-016): - Dry-run with pending changes: manifest.json content AND mtime unchanged, config.json unchanged, zero upload calls, zero AllFiles calls. Notes in comments what remains for staging validator (real mtime across a process invocation). interruption_test.go (VAL-CROSS-011): - Simulated mid-upload interruption via fault injection: first sync exits non-zero without advancing state, rollback directory remains; second sync restores stale rollback via Phase 0 and converges to a manifest matching the fake's server state. Notes in comments what remains for staging validator (real SIGINT delivery). Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… so config never advances ahead of manifest Phase 6 previously wrote config.json before manifest.json. A SaveManifest I/O failure (reachable in normal operation) left config advanced past a stale manifest — the manifest-poisoning shape this mission exists to close: the next sync sees no drift, fast-paths, copies the stale BASE to REMOTE, and reports "Up to date." forever. Reorder to buildNewBaseManifest → SaveManifest → SaveConfig. The failure direction is now safe: a SaveConfig failure leaves the manifest advanced and config stale, so the next sync detects drift, fetches AllFiles, and rebuilds BASE from real remote data — self-healing. The reorder is data-safe: nothing between the two writes reads config from disk. Flip TestMissingSentEntry_Phase6DoesNotAdvanceManifest to assert config is NOT advanced (was asserting it IS advanced as a known hazard). Add TestSaveManifestFailure_DoesNotAdvanceConfig and TestSaveConfigFailure_ManifestAdvanced_NextSyncResyncs to guard both failure directions. The write pair is still not transactional: each write is individually atomic via fsutil.AtomicWriteFile, but a crash between two successful writes still leaves one advanced. This residual risk is documented in library/phase6-write-ordering.md for the findings writeup. No transactionality mechanism is introduced. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…tion Add regression tests that would fail loudly if a size+mtime fast path (the mechanism RAPTOR-19525 alleged) were ever introduced: - TestSameSizeSameMtimeDetected: rewrites app.py to different bytes of identical length with mtime restored via os.Chtimes, asserts it appears in the upload plan — the codified refutation of the ticket's claim. - TestDetectionIsContentOnly: 2x2 matrix over {size matches manifest, size differs} x {content matches, content differs}, asserting detection fires iff content differs in every cell. - TestDetectionControls: true-positive controls (size+content change, 4 MiB large-file change) and false-positive controls (untouched file, mtime-only advance). - TestPlanRowSizesMatchDisk: each upload plan row's displayed byte size equals the real on-disk size at plan time. - TestClassifyTruthTable: comprehensive table over every (base, local, remote) hash triple covering all fourteen classifications and their mapped actions, transcribed from the current Classify implementation. - TestClassifyActionMappingIsExhaustive: every classification has a defined non-default action. Every detection test was proven to fail against a deliberately stubbed size+mtime fast path before being accepted. The false-positive controls were proven to fail against a mark-everything-dirty stub. The truth table was proven to catch a changed action mapping. No production code changes. Only *_test.go files added. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…sion Add regression coverage for the behaviour immediately surrounding the upload-integrity fix so it cannot regress silently. Covers ignore rules (.drignore exclusion, .wapiignore legacy shadow warning, system excludes, case-folding), case-collision detection before any upload, path normalization (forward slashes, NFC, no leading ./ or trailing slash) in both manifest build and base/remote comparison, plan action mapping (local-deleted → upload-delete, remote-modified → download, both-sides → conflict, remote-wins resolution), dr workload up's code-change measurement (modified tree count, unchanged tree zero, first-deploy flag for unlinked project), the sync lock (second concurrent sync rejected, stale-rollback recovery, forward state migration). 22 new test cases across two files. Every representative test was proven to fail against a deliberately broken production line before being accepted. Full task test green; task lint clean on linux, darwin, and windows. No production files touched. Fulfills VAL-REGRESSION-007, VAL-REGRESSION-008, VAL-REGRESSION-009. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…form field The zip-path incremental sync (UploadFromZipExisting) sent overwrite=REPLACE as a URL query parameter, but the server's /files/<catalogID>/fromFile/ route binds its validator fields from the multipart form body only and silently defaults to RENAME when the form field is absent. Every re-uploaded path was therefore stored as "name (2).ext" while the original path kept its stale bytes, and the manifest recorded the new streamed hash at the original path — so every later sync falsely reported "Up to date." while the server served old content. Move overwrite into the multipart form body. newStreamingMultipartRequest now frames extra form fields as complete parts BEFORE the file part (fields-first lets streaming parsers collect parameters without buffering the file), with field names sorted for deterministic framing; Content-Length stays exact because the fields fold into the prologue (verified byte-for-byte by test). useArchiveContents stays in the query string: the monorepo validator (MLData/files/public_api/files/validators.py, AddFilesIntoContainerBaseValidator) declares use_archive_contents as a form field whose default is 'True' — the server defaults to archive extraction and never reads request.args, so the query flag is ignored and extraction happens regardless. Moving it would change nothing observable; the test pins that decision. UploadFromZipNew is unchanged apart from the shared framing helper: a first sync has no pre-existing paths, so the server's RENAME default is correct and it sends no overwrite field. Verified end to end against staging: 25-file first sync (zip path), 22-file incremental re-sync (zip path) — all 22 re-uploaded paths hold the new bytes (disk shasum == manifest hash == server fileChecksum), zero "name (2).ext" entries, unmodified controls intact, and a following plain --dry-run truthfully prints "Up to date.".
…hree regression test files The Copyrights CI check flagged surroundings_regression_test.go, upload_failure_test.go, and codechange_regression_test.go. Each carried a header that was silently corrupted during an edit: two had the license URL truncated to licenses/2.0 and one had dropped "in" from "except in compliance with the License". All three now byte-match the canonical header used by their sibling test files, and license-eye reports 0 invalid files.
…the Serving signature TestRun_WizardRedirectIsFollowed still wired the wait fake with the pre-#844 signature, so the package stopped compiling after the rebase onto main. Upstream main carries #844's workload.Serving parameter and #848's wizard tests but never reconciled the two, so this breakage exists on main itself and was inherited by the rebase. The literal now matches its sibling at TestRun_NoManifestOnATerminalRunsTheWizard; the run always passes workload.Serving{} today, which is what the test ignores. This commit repairs upstream main and can be dropped on the next rebase if main lands its own fix first.
…try, before any state write phase6State assigned its rollback.Discard() to the very bottom, after SaveManifest and SaveConfig. A SaveConfig failure returned early and stranded the rollback dir holding pre-sync bytes. The next run's Phase 0 (RestoreStaleIfPresent) blindly copied those bytes back into the working tree; against the manifest the failed run had already advanced, the diff classified them as local edits, and the sync silently re-uploaded them over the remote — a self-perpetuating corruption chain with no prompt. Discarding at Phase 6 entry is safe: e.rollback is only assigned after executePlan succeeds (a Phase 5 failure restores and returns), Phase 6 never restores because the remote has already advanced, and discarding unconditionally makes cleanup independent of state-write success. Regression tests cover all three Phase 6 outcomes (clean success, SaveManifest failure, SaveConfig failure) plus the two-run corruption chain: fault-injected SaveConfig failure leaves no rollback dir, and the next run performs no stale restore and produces zero false LOCAL_MODIFIED uploads for rollback-covered paths. The mid-Phase-5 interruption behavior is unchanged (rollback dir still strands and the next run still restores). The tests were demonstrated to fail against the pre-fix ordering. The fake files client gains opt-in downloadable content so engine tests can exercise a plan with downloads, which is what puts real pre-sync bytes into the rollback tree.
…ame the specific error
ajalon1
force-pushed
the
aj/raptor-19525-changed-files
branch
from
August 29, 2026 06:24
68a420e to
066dde1
Compare
ajalon1
added a commit
that referenced
this pull request
Aug 29, 2026
… windows_crossplatform_test.go The header read 'distributed on an "AS IS IS" BASIS' — a duplicated IS introduced during the rebase, which failed the CI Copyrights check on PR #868. Restored the canonical Apache-2.0 wording byte-identical to its sibling files. Comment-only; verified with license-eye header check over the whole tree (invalid: 0), go vet, and the full task test suite. Lefthook bypassed per repo docs for comment-only commits; go vet and the full suite were run manually in its place.
… fails Phase 6 discarded the rollback at entry but swallowed the Discard error (`_ = e.rollback.Discard()`). If RemoveAll itself fails, the rollback dir survives next to ADVANCED state, and the next run's stale-rollback recovery resurrects pre-sync bytes that the advanced manifest misclassifies as local edits and silently re-uploads over the remote — the strand/stale-restore/ false-upload chain, reopened through the Discard-failure path. Surface the error instead. At Phase 6 entry nothing has been persisted, so a wrapped error is the safe direction: the next run keeps the rollback dir AND un-advanced state, which is the recoverable mid-Phase-5 outcome — the stale restore puts back bytes the un-advanced manifest still matches, and the next diff schedules downloads, not false uploads. The discard is extracted into discardRollback so the phase's own failure contract stays explicit. Fault-injection test: a hook on Phase 5's final step (PatchCodeRef) strips write permission from the rollback dir between rollback creation and Phase 6, proving a Discard failure aborts with the wrapped error before SaveManifest — manifest byte-identical, config un-advanced, rollback dir still present. Also drop the now-unused //nolint:gosec on the config-restore write: gosec's taint analysis no longer flags that site once the package gains the new test, and nolintlint rejects directives that suppress nothing. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ed setup The discard-failure test's t.Cleanup closure dereferences s.rollDir to restore the read-only permissions. If scenario setup itself fails, the scenario (and its rollback dir) may never have been assigned; guard the closure so the cleanup cannot nil-deref during teardown of an already failing test.
… pin the .wapiignore shadow warning Close two test-infrastructure gaps that milestone-1 scrutiny found, both of which blocked honest assertions in later milestones. Test scaffolding only; no production behaviour changes. GAP 1 - the fake could not model downloads: DownloadFile was a hard error stub, so nothing could test a download actually landing on disk, and the remote-wins coverage could only assert plan structure (conflict.RemoteHash). - Implement DownloadFile against the in-memory server model: it serves the exact bytes recorded for the requested path at the requested version, via a per-version content store threaded through the stage path, both zip paths, and remote deletes. Hash-only withVersion seeds stay non-downloadable and say so instead of serving empty bytes. - Add per-path fault hooks: withFailDownload (download errors) and withCorruptDownload (same-length bytes whose hash differs from the advertised checksum, so the client-side post-download checksum verification is what catches the corruption, not the size check). - Add a DownloadFileCalls counter alongside the other per-method counters. All download state is mutex-guarded; the suite stays clean under -race. - Upgrade TestPlanAction_RemoteWinsResolution from plan-structure-only to executing the resolution: the remote bytes land on disk, hash to the server's checksum, the .LOCAL. copy keeps the local bytes, and the manifest records the remote hash. - New download_path_test.go pins the downloads-only plan shape: the download writes the advertised bytes, a failed download rolls back the tree, and a checksum mismatch fails without keeping corrupt bytes. GAP 2 - the .wapiignore shadow warning had no capture seam: it is emitted only via log.Warn in phase2_manifests.go, so the both-files-present test could assert only that IgnoreFileNotice is empty. - Add captureWarnLog (log_capture_test.go), mirroring the repo's existing stderr-pipe capture pattern in internal/tools rather than inventing a mechanism, and pin the exact warning text as wantShadowWarning. - Strengthen TestWapiignoreShadowWarning_BothPresent to assert the warning actually fires, with a false-positive control proving no warning fires when only the current filename is present. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…d into engine options and telemetry Introduce the --verify flag end to end without changing any sync behaviour beyond carrying the option. sync.Options gains Verify, the sync command registers a --verify boolean whose usage states it forces a remote round-trip and post-apply verification of what was uploaded, and the value is threaded through runFlags into the engine Options. The flag is transient: read directly from cobra, never bound into viper and never persisted to drconfig.yaml. It deliberately does not join the dry-run/diff mutual-exclusion group, and it must never fold into previewOnly: a non-dry-run verify run still applies its plan, and tests at both the command and engine level guard that boundary. The sync telemetry event now emits a verify attribute unconditionally (false without the flag) so verify runs are distinguishable from plain runs. Composes with --dry-run, --diff, --yes and --output-format json with no flag-conflict error. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… --verify Phase 2's non-drifted fast path copied BASE into REMOTE, so a manifest that stopped describing the server was permanently invisible: the plan printed "Up to date." while the server held different bytes. --verify now bypasses the fast path (in dry-run and non-dry-run alike), fetches the real AllFiles listing, and compares BASE against it per path, distinguishing a hash mismatch from each one-sided absence. The findings are diagnostics, not errors: exit code stays 0, and the plan changes only because the real remote is now known (which produces the reconciling rows). Each finding is logged from Phase 2 via log.Warn so it survives a later phase failing, summarized by runSync alongside the other state notices, and rendered as a structured, always-present "divergence" field on PlanJSON whose entries carry the path, the kind of divergence (hash_mismatch / base_only / remote_only), and both hashes. Drifted artifacts are deliberately exempt: there the remote is a newer version by design, so BASE-vs-REMOTE differences are ordinary drift, not findings. The default path makes zero AllFiles calls and emits nothing, exactly as before. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…al remote even when the plan is empty A --verify run that records BASE-vs-REMOTE divergence must reach Phase 6 even when the plan comes back empty. The sharpest shape — BASE poisoned to hash A while disk and server both hold B — classifies as CONVERGED, every row is skipped, and the plan is empty; the manifest rewrite in Phase 6 is then the only repair that happens. Both empty-plan short-circuits (Engine.Run and the command layer's skip-Execute decision) now fall through when divergence findings are non-empty, routing through Execute where Phase 5 no-ops and Phase 6 seeds the new manifest from the real remote Phase 2 fetched — dropping the poison and any BASE-only phantom paths. Previews never repair, and runs without divergence findings keep the pinned empty-plan short-circuit. The history entry now records the version the run ended on rather than the raw new-version ID, so a run that applies no uploads logs "x→x" instead of a truncated "x→". Engine tests cover the empty-plan repair (converged hash mismatch, plus a BASE-only path that is also locally deleted), the flagship download repair and its idempotency, the conflict preview under --dry-run, the locked- artifact gates under --verify, the untouched default path, and byte-level equality of the persisted state against a plain sync (no verify field ever persists). Command tests pin that an empty plan with divergence findings still Executes in both human and JSON modes.
…ds what was uploaded Under --verify, Phase 5 now fetches AllFiles for the newly created version after ApplyUploads and compares each uploaded path's server checksum against the hash streamed during the upload. A mismatch fails the phase, trips the existing rollback, and stops the pipeline before Phase 6, so neither manifest.json nor config.json can advance past a version the server does not actually hold. Only uploaded paths are compared: stage REPLACE merges staged paths in place, so the listing legitimately contains files this sync never touched, some carrying checksums from earlier runs. ApplyStage's numFiles counts every file in the resulting version, not the ones uploaded, so it never gates the check. A plan with no uploads (downloads-only or empty) skips the check entirely and makes no AllFiles call; a nil outcome or empty Sent map is not an error. The check runs after the codeRef PATCH deliberately: the remote version cannot be un-published, so the recoverable posture after a failed verification is the drifted one, where the next sync fetches the real remote and reconciles. Tests: fake-based engine cases for the mismatch (error naming the path, both state files byte-identical to pre-sync), the dropped-path case, the only-uploaded-paths boundary (via a new version-scoped checksum hook on the fake), the numFiles boundary, and the no-upload skips; plus an httptest-level test driving the check through the real httpClient against a 2-page listing whose uploaded path exists only on page 2, since pagination lives below the Client interface where the fake cannot reach. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… symlinks at collection site Extend SymlinkLogger with an isDir boolean resolved via os.Stat (which follows the link), so the caller can distinguish a skipped file from a skipped directory subtree. A dangling symlink reports an empty target and isDir false with no walk error. A symlink chain is not followed: each top-level link is reported once. Filter every reported symlink through the same ignore matcher and system-exclude set used for regular files, at the collection site in phase2_manifests.go. The walk's symlink arm returns before the ignore check, so without this filter a symlink matching a .drignore pattern or a system-excluded name (e.g. .git) would still be announced — the classic unfiltered-warning trap. Add walk-level tests for the symlinked-directory pruning case, isDir resolution, dangling symlinks, symlink chains, and external targets, skipping on Windows following the existing runtime.GOOS precedent. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ia stderr and PlanJSON Collect the filtered symlink reports in Phase 2, expose them on the engine via SkippedSymlinks(), and emit an actionable notice through log.Warn from within Phase 2 so it survives a later phase failure. The notice says the symlink was NOT uploaded or synced (not merely that one was found), with distinct wording for file vs directory symlinks — the directory wording conveys that a whole subtree is omitted. Add an always-present, explicitly-empty-when-empty skippedSymlinks field to PlanJSON (mirroring the locked:false precedent), distinct from the uploads/downloads/deletes/conflicts arrays. The stderr prose is bounded at SymlinkNoticeBound (5): when more than 5 symlinks are skipped, stderr lists the first 5 in deterministic order followed by a count of the remainder, while the JSON field lists every one. Wire the rendering path end to end: the engineRunner interface gains a SkippedSymlinks accessor, realEngine and fakeEngine implement it, runSync emits the notice to stderr alongside the existing notices, and finishJSON threads the symlink data into RenderPlanJSON so the new PlanJSON field is populated. The notice fires in every mode (--dry-run, --diff, real sync) and even when the plan is empty, never changing the exit code from 0. A symlink path never appears in any action list. For a project with no symlinks, the human output is byte-identical to the pre-change build and the JSON plan differs only by the added empty skippedSymlinks field. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ned integration scenarios Add engine-level and cmd-level integration tests covering scenarios where hash-while-streaming, --verify divergence detection/repair, and symlink surfacing all fire in a single run. Each test exercises multiple fixes at once through Plan/Execute against the self-consistent fake, proving they compose without interfering. Engine-level tests (internal/workload/sync/combined_integration_test.go): - Symlink + mid-stream rewrite: both fixes fire, symlink omitted from uploads, raced file holds the streamed hash - Symlink + poisoned BASE + --verify --dry-run: both structured fields populated, both notices on stderr - Symlink-replacement delete under --verify: NOT reported as divergence (false positive would train users to ignore the notice) - Zip-path (21+ files) + divergence + both symlink kinds: streamed hashes match server, repair holds, neither symlink enters the archive - Maximal mixed plan (upload, download, delete, conflict, file symlink, directory symlink, divergence): every field populated, every notice on stderr, manifest matches server, exit 0 - All notices at once (shadow warning, symlinks, divergence): no notice suppresses another - Default no-verify UX across first sync, no-change re-sync, mid-sync edit, symlink replacement, and mixed plan: correct hashes, no divergence notices, no extra AllFiles round-trip - Exit-code coherence: 0 for diagnostics-only, non-zero for genuine failures Cmd-level tests (cmd/artifact/code/codesync/combined_integration_test.go): - JSON mode with symlinks + divergence: stdout pure JSON, both fields populated, both notices on stderr - Maximal mixed plan in JSON mode: every field populated, pure stdout - All notices in JSON and human mode: no notice suppresses another, each stream independently reconstructable Staging-facing sequences recorded in library/combined-scenario-staging-sequences.md. Fulfills VAL-CROSS-004, VAL-CROSS-006, VAL-CROSS-007, VAL-CROSS-008, VAL-CROSS-009, VAL-CROSS-010, VAL-CROSS-012. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…parity, path formatting, and no-symlink sync Add cross-platform unit tests covering the Windows-specific behaviours requested on the ticket, scoped to unit tests only (no Windows e2e run): - SHA-256 parity: assert fileops.HashFile and the streaming hash produce identical SHA-256 for content with CRLF line endings, mixed endings, binary \r, and trailing \r — guarding against any future text-mode translation that would silently change hashes on Windows. - Path formatting: assert plan output paths and manifest.json keys use forward slashes on all platforms, identical to POSIX. On Windows the OS-native walker produces backslash-separated paths; NormalizePath converts them. The test runs everywhere and is load-bearing on Windows. - No-symlink project: assert a project with no symlinks produces no symlink notice and syncs normally — the no-symlink code path is platform-independent. - Platform-inappropriate check skips: file-mode assertions skip on Windows with a visible reason (NTFS collapses mode bits); the sync lock cross-process exclusion test skips on Windows with a visible reason (synclock_windows.go is a no-op). Both follow the existing runtime.GOOS == "windows" precedent. Every test was proven to fail against a deliberate fault before being accepted: SHA-256 parity by stripping \r in hashFile; path formatting by replacing / with \ in NormalizePath; no-symlink by injecting a fake skippedSymlink. All faults were reverted; only *_test.go files are modified. Fulfills VAL-SYMLINK-012 and VAL-REGRESSION-013. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…gged weaker-than-claimed assertions Close the test-quality gaps catalogued as non-blocking, so no test keeps passing for a weaker reason than its name or feature text claims: - Partial-upload failure tests (worker error, one-late-failure) now assert the error names SOME planned upload path (set membership) instead of merely containing "upload"; which path loses the race is nondeterministic under 4-way concurrency. - The deleted-file-between-plan-and-execute test asserts UploadToStageCalls directly (upper bound; in-flight workers may finish) instead of proxying "nothing applied" through ApplyStageCalls. - The interruption convergence test adds set-size equality between the manifest and the fake's AllFiles, so a path missing from or extra on either side is caught in both directions. - New end-to-end case-collision test drives Plan with a genuinely colliding tree and asserts the failure precedes every upload-side call; it skips with a visible reason on case-insensitive filesystems, so Linux CI exercises it for real. - The dry-run positive control reads the plan Run already computed instead of re-running Plan(), removing the latent coupling to plan-phase purity. - Fake footguns closed/documented: withVersion now copies the caller's map (pinned by a self-test) and CreateStage's single-active-stage staging-area wipe is documented at the definition site. - The Phase 6 cfg := e.config value-copy invariant is pinned by a test that fails if Phase 6 ever mutates a shared pointer target in place, with the reference-type-field hazard documented. - The readZipEntries doc comment ending is fixed, and the overclaiming TestCaseCollision_ErrorIsBeforeUpload is renamed to TestPlanWithoutCaseCollisions_MakesNoUploadCalls to match what it verifies. Every strengthened assertion was proven to fail against a deliberate regression (path-wrap removal, upload-anything-on-open-failure, dropped remote seeding, aliasing restored, in-place pointer mutation) and all temporary mutations were reverted; only *_test.go files are changed.
Add the missing regression coverage for the state around the sync engine: - temp_cleanup_test.go (new): after a sync completes, whether it succeeded or failed, no wapi-sync-*.zip may remain in the system temp dir and no AtomicWriteFile <name>.tmp.* litter may remain in the state dir, for both the stage path and the zip path. The zip-failure case drives buildZip into its error path with the archive temp already on disk (a planned file is deleted between Plan and Execute). The system-temp assertion compares before/after snapshots rather than absolute contents, so a stray from an older build does not fail it and a concurrent creator cannot pass it vacuously. The rollback tree is asserted gone on success and present after failure on purpose: a failed sync leaves it as the recovery copy the next run's Phase 0 restores and removes, which the interruption tests already pin. - checkout: the .tmp-<version>-* staging dir must be gone from the checkouts parent after both a successful checkout (renamed away) and a failed one (removed by the failure defer). - versions: the "Local synced to: <id>" line renders in text mode when config.lastSyncedVersionId is set, and is omitted when the project has never synced. The cleanup tests were verified to have teeth by disabling the zip temp removal and watching TestSyncCleanup_ZipPathSuccess fail naming the leftover wapi-sync-*.zip.
… from newStreamingMultipartRequest Every caller already built query parameters into the request URL before calling, so the query url.Values parameter was always nil and the len(query) > 0 branch was dead. Remove the parameter and the branch, and document that requestURL is used as-is. The generated requests are byte-identical: the existing multipart wire-format tests (fields-first framing, Content-Length parity, overwrite placement) pass unchanged, and stage- and zip-path syncs against staging still produce manifest == disk == server checksums.
…e Phase-6 config copy site Mirror the value-copy safety invariant pinned by TestPhase6ConfigCopyIsNotMutatedInPlace into phase6State itself: the cfg := e.config value copy is safe only while wapi.Config has no reference-type field that Phase 6 mutates in place; if such a field is ever added, the copy must become a deep copy. Comment-only change, no behavior change. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…y scrutiny nits Seven non-blocking nits from the symlink-visibility scrutiny synthesis: - phase2_symlink_test: table the directory-symlink .drignore case over the trailing-slash and bare pattern spellings, so the matcher's bare-pattern branch is asserted where the claim is made - combined_integration_test: assert manifest==server for all 23 zip-path uploads (realfile.py and realdir/inner.py were omitted), and rewrite file_10.py between Plan and Execute so the streamed-hash claim is independently discriminating on the zip path too - combined_integration_test: delete the redundant immediately-overwritten poisonManifestHash call and its self-correcting comment in the VerifyDryRun fixture - windows_crossplatform_test: drop the hand-rolled itoa helper; descriptive subtest names remove the need for it entirely - windows_crossplatform_test: pin the SHA-256 parity expectations to hardcoded hex digests cross-verified with shasum and python hashlib, so the expected values are externally eyeball-verifiable - surroundings_regression_test: remove the case-probe directory before fsIsCaseInsensitive returns so the probe never leaks into the fixture - temp_cleanup_test: pin TestSyncCleanup_ZipPathFailure's error to buildZip with require.ErrorContains(err, "for zip") Teeth proven by temporary working-tree mutations, since reverted: Phase 6 recording the planned hash fails the new zip streamed/planned assertions; dropping the collection-site symlink filter fails both table cells; "for zip" removed from the buildZip open error trips the new pin; a corrupted digest fails the parity test naming the case. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…bout what the run writes The one-line divergence summary claimed "The plan reconciles them." in every mode, and the empty-plan repair run printed "Up to date." on stdout immediately before the Execute that repairs manifest.json; both read as though nothing needed fixing. divergenceSummaryNotice is now mode- and plan-aware: previews state that nothing was written and name the flag to rerun without; the empty-plan repair names the Phase 6 manifest rewrite from the server's state as the reconciliation; non-empty applying runs keep the existing wording. The human-mode repair path prints an honest line via display.PrintEmptyPlanRepair instead of "Up to date."; previews and genuinely converged empty plans keep "Up to date.". The load-bearing per-path divergence notices in divergence.go are unchanged, and no behavior, exit code, or JSON shape changes. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…erify, and symlink notices
Extend the `dr artifact code sync` reference with the integrity behaviors
introduced in the parent mission:
- Note that the zip upload requests overwrite=REPLACE on the multipart
form, so re-syncing an existing artifact replaces the prior version
instead of the server renaming the upload to `name (2).zip`.
- Document --verify: forces a remote round-trip before planning even
when the artifact has not drifted, surfaces BASE-vs-REMOTE differences
as `divergence` entries in the plan and plan JSON, repairs the
manifest from the server's real state during execution, and
re-verifies uploaded checksums after apply; composes with --dry-run
and --diff.
- Document symlink visibility: each skipped symlink prints a
kind-differentiated stderr notice bounded at five paths with an
"and N more" tail, the plan JSON always emits `skippedSymlinks` (`[]`
when none), and symlinks excluded by .drignore or by the hardcoded
system excludes are not reported.
Verified against cmd/artifact/code/codesync/cmd.go,
internal/workload/sync/{engine.go,phase2_manifests.go,phase5_execute.go,symlink.go,upload_zip.go,upload_stage.go},
and internal/workload/sync/display/json.go (field name is `divergence`,
singular).
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… version-content download model
… windows_crossplatform_test.go The header read 'distributed on an "AS IS IS" BASIS' — a duplicated IS introduced during the rebase, which failed the CI Copyrights check on PR #868. Restored the canonical Apache-2.0 wording byte-identical to its sibling files. Comment-only; verified with license-eye header check over the whole tree (invalid: 0), go vet, and the full task test suite. Lefthook bypassed per repo docs for comment-only commits; go vet and the full suite were run manually in its place.
…removal so Windows cannot strand it buildZip removed its wapi-sync-*.zip temp while the file handle was still open. Go opens Windows files without FILE_SHARE_DELETE, so that remove fails with a sharing violation and the discarded error strands the archive in the system temp dir on every zip-build failure — observed as a persistent TestSyncCleanup_ZipPathFailure failure on the windows-latest CI leg. The temp is now closed on every path before the failure-path removal, with the archive writing factored into writeZip; on POSIX the observable behaviour is unchanged. TestCombined_ExitCodeCoherence/case_collision_nonzero required a non-zero exit for a case collision, but the collision must exist on disk for Run() to see it and a case-insensitive filesystem (macOS APFS and Windows NTFS by default) collapses APP.py into app.py — the subtest is now filesystem-aware via the same probe the collision tests already use. The detector itself stays covered platform-independently by TestCaseCollision_FailsBeforeUpload.
…sertion Two hygiene leftovers from the version-content download rework: - fake_files_client_test.go: the comment block describing the removed downloadContent field (and its opt-in "not expected" failure mode) outlived the field itself; the live content store is versionContents, which documents itself. Delete the orphan. - path_safety_test.go: the NotContains against "DownloadFile not expected" asserted the absence of a message no code can emit anymore. The Contains assertion on the unique unsafe-path wrapper already proves SafeRelPath fired before any remote call, so the dead assertion is removed without repointing.
…d-writer contract The buildZip failure path discarded the os.Remove error, so a temp archive that failed to delete (the exact Windows sharing-violation class the close-before-remove fix addressed) vanished silently and a recurrence would be undiagnosable. Log the removal error at debug level instead. Also pin, with a comment on the addToZip error path, that writeZip leaves the zip writer deliberately unclosed: the caller closes the temp file and removes the dead archive, so closing here would only flush a doomed central directory and risk masking the real failure.
ajalon1
force-pushed
the
aj/raptor-19525-changed-files
branch
from
August 29, 2026 20:42
a65530e to
6c3ea31
Compare
wojtekwdr
requested changes
Aug 31, 2026
|
|
||
| switch { | ||
| case !ok: | ||
| out = append(out, Divergence{Path: path, Kind: DivergenceBaseOnly, BaseHash: b.Hash}) |
Contributor
There was a problem hiding this comment.
[high] A base_only finding says BASE is wrong about this path, but Classify still trusts BASE for it: non-drifted artifact, local matching BASE, path missing from the remote gives ClsRemoteDeleted, and Phase 5 removes the user's only copy with no conflict copy and exit 0. I reproduced it with the fake's own withDropPath: one plain sync against a server that drops an upload, then sync --verify, and the file is gone. divergence_test.go:373 pins it as intended, so is "remote wins" really right when the run has just proved the remote never held it?
| case plan.IsEmpty(): | ||
| return head + " The plan is empty, but manifest.json is being rewritten from the server's state to repair them." | ||
| default: | ||
| return head + " The plan reconciles them." |
Contributor
There was a problem hiding this comment.
[medium] This is printed in runSync before the conflict prompt, so quitting at the prompt, or JSON mode returning early on conflicts without --yes, leaves "The plan reconciles them." on stderr for a run that executed nothing. The dry-run, diff and empty-plan branches are all careful about exactly this; the conflict-abort mode is the one that got missed.
| require.NoError(t, err) | ||
|
|
||
| origStderr := os.Stderr | ||
| os.Stderr = w |
Contributor
There was a problem hiding this comment.
[low] os.Stderr = w has no defer, and every call site passes a closure containing require.*. The first failure Goexits past the restore, past w.Close() and past the t.Cleanup(log.StopStderr) registration, so the rest of the package logs into an orphaned pipe and loses its warn output exactly when you need it.
| assert.Contains(t, outText, "a.py", "stdout must carry the plan") | ||
| assert.NotContains(t, outText, "Moved local state", "no migration notice on stdout") | ||
| assert.NotContains(t, outText, ".wapiignore", "no ignore notice on stdout") | ||
| assert.NotContains(t, outText, "was not uploaded", "no symlink notice on stdout") |
Contributor
There was a problem hiding this comment.
[low] These two negatives cannot fail. "was not uploaded" and "divergence:" are the phase wording from symlink.go and divergence.go, which reach the real stderr via log.Warn and never appear at all under fakeEngine; the cmd summaries say "were not uploaded or synced" and "--verify found N divergence(s)". Routing format.StateNotice to stdout leaves both green. The "Moved local state" negative beside them does have teeth.
| // false so the caller can label it without a walk error. | ||
| info, err := os.Stat(absPath) | ||
| if err != nil { | ||
| onSymlink(relPath, "", false) |
Contributor
There was a problem hiding this comment.
[low] A directory symlink whose os.Stat fails reports isDir false, so phase 2 matches it as a file and announces a link the user ignored: node_modules/ only matches with isDir true, so a dangling node_modules link gets warned about anyway, and with the "not a regular file" wording. Splitting fs.ErrNotExist from the other Stat errors would keep the file-vs-directory contract intact.
| // original leak — so a discarded error here would hide exactly the | ||
| // recurrence this cleanup exists to prevent. Log it instead. | ||
| if rmErr := os.Remove(tmp.Name()); rmErr != nil { | ||
| log.Debug("zip temp cleanup failed; the archive may be stranded in the system temp dir", |
Contributor
There was a problem hiding this comment.
[low] The comment just above says a discarded error here would hide the recurrence this cleanup exists to prevent, then it logs at log.Debug, which is off at the default level for both the stderr and the file logger. engine.go:313 uses log.Warn for the comparable lock-release cleanup failure.
|
|
||
| e.divergences = detectDivergence(e.base, e.remote) | ||
|
|
||
| for _, d := range e.divergences { |
Contributor
There was a problem hiding this comment.
[low] nit: one log.Warn per divergence with no bound, and divergenceSummaryNotice joins every path into a single line, while the symlink notices beside them cap at SymlinkNoticeBound with an "and N more" tail. A wholesale-poisoned manifest is the case --verify exists for, and it is the case that prints a line per file plus one very long one.
| interactive directory prompt. | ||
| write. --verify forces a remote round-trip and post-apply verification | ||
| of what was uploaded, catching server-side divergence the ordinary fast | ||
| path cannot see; it composes with --dry-run and --diff. --yes |
Contributor
There was a problem hiding this comment.
[low] "post-apply verification of what was uploaded ... it composes with --dry-run and --diff" reads as though the post-apply half runs under a preview, but skipsExecute returns true there so verifyPostApplyUploads is never reached. Worth splitting the two halves: the round-trip composes, the post-apply check only happens on an applying run.
captureWarnLog swapped process-global os.Stderr with no defer, so a require.* FailureNow inside the passed closure unwound via Goexit and skipped the stderr restore, the pipe close, and the StopStderr cleanup. Register the restore in a t.Cleanup before invoking fn so it runs even after FailNow. The two human-mode stdout negatives pinned per-phase prose the fakeEngine never emits, so a StateNotice misrouted to stdout would have passed. Pin the actual command-summary substrings instead. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…gging, and --verify notices - Dangling symlink filtering: split fs.ErrNotExist from other os.Stat failures on the walk so a dangling link carries a dangling flag, and check both file and directory ignore-pattern spellings at the Phase 2 collection site — "node_modules/" now filters a dangling node_modules link instead of warning "not a regular file". - Zip temp cleanup: log the removal failure at Warn instead of Debug so the stranded-archive recurrence is no longer invisible at the default level, matching the lock-release cleanup. - Divergence notices: bound per-divergence Warn output and the joined path list in the cmd summary at DivergenceNoticeBound (5) with an "and N more" tail, while the plan JSON still carries the full list. - --verify help: split the remote round-trip half (composes with --dry-run/--diff) from the post-apply re-fetch, which only runs on an applying run. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
RATIONALE
Stacked on the integrity-fixes PR (base:
aj/raptor-19525-integrity-fixes). That PR fixed how the CLI writes state; this one answers "how do I find out my manifest is already lying to me?" and "why did my file never make it to the server?"dr artifact code sync --verifyforces a remote listing (AllFiles), reports any path where BASE diverges from REMOTE (hash_mismatch/base_only/remote_only) as precise per-path stderr notices, repairs the manifest from the real remote (including the empty-plan case, where the plan has nothing to do but the manifest still needs rewriting), and — after applying — re-fetches the new version and compares each uploaded path's server checksum against the bytes actually streamed, failing before any state is persisted on mismatch. Exit code stays 0 when divergence is found and repaired; diagnostics never fail a sync.--verifyis deliberately opt-in (one or two extraAllFilesGETs) and never folds into--dry-run/--diffsemantics; nothing is persisted that would distinguish a verified run.Skipped-symlink surfacing: symlinks have always been skipped silently, so a file absent from the remote was invisible. The walker now reports each skipped symlink with its kind (file vs directory, resolved via
os.Stat; dangling links reported with empty target), filtered through the same ignore matcher and system-exclude set as regular files (a.drignore-matched or.gitsymlink is neither uploaded nor announced). Phase 2 emits a bounded, deterministic stderr notice (first 5, then "and N more"; distinct file vs directory wording; directory wording conveys the whole subtree is omitted), andPlanJSONgains an always-presentskippedSymlinksfield (explicitly[]when empty, mirroringlocked: false). Human output for a project with no symlinks is byte-identical to before.CHANGES
d53db1ectest: model downloads in the fake client and pin the.wapiignoreshadow warningd33e4e25feat: add--verifyflag, carried into engine options and telemetry15f1be5ffeat: detect BASE-vs-REMOTE divergence under--verify3316d3eafeat: repair a poisoned manifest from the real remote even when the plan is empty43bdf65cfeat: verify after apply that the server holds what was uploaded42874320feat: report symlink kind and filter ignored symlinks at collection site3cb231d5feat: surface skipped symlinks via stderr and PlanJSONecad32bftest: prove the three fixes compose in combined integration scenarios9ec96a0ctest: add Windows unit coverage for SHA-256 parity, path formatting, and no-symlink sync2f80eac2test: harden test quality where scrutiny flagged weaker-than-claimed assertionse7baa205test: pin sync temp cleanup and checkout/versions surroundings51b07b8arefactor: drop the vestigial query parameter from newStreamingMultipartRequest9a35a9c0docs: state the shallow-copy invariant at the Phase-6 config copy site05fb1263test: sweep the actionable symlink-visibility scrutiny nits2723cb15fix: make--verifydivergence prose honest about what the run writes68a420e4docs: document sync overwrite semantics,--verify, and symlink noticesTESTING
task testandtask lintgreen; the--verifyhalf was validated end-to-end against live staging (26/26 behavioral assertions), including: flagship poisoned-manifest scenario reproduced (Up to date.lie) then caught and repaired by--verifywith exit 0; idempotent repair (second--verifysilent, manifest byte-identical); post-apply verification cost pinned at exactly twoAllFilesGETs for a non-drifted run with uploads; JSON mode pure on stdout to EOF with all prose on stderr.--verifyrepair, and symlink surfacing compose in a single run (including a 22-file zip-path project with a divergence and both symlink kinds).NOTES
Note
Medium Risk
--verifychanges when sync executes and can fail runs on post-apply checksum mismatch or rewrite manifest state; default sync behavior is intended unchanged, but the new repair and verification paths touch core sync state handling.Overview
Adds
dr artifact code sync --verify, an opt-in integrity mode that forces a remote listing on non-drifted artifacts, reports BASE (manifest) vs REMOTE mismatches on stderr and in plan JSON (divergence), and still applies when needed—including empty plans that only repair a poisoned manifest via Phase 6 instead of stopping at “Up to date.” Human and JSON output keep diagnostics on stderr; JSON plan documents always include explicitdivergenceandskippedSymlinksarrays (empty when clean). Telemetry always emitsverify.Skipped symlinks are no longer silent: the walker passes file vs directory (and dangling links) into the sync engine, stderr gets a bounded, sorted summary, and symlinks ignored by
.drignore/system excludes are not announced.Smaller surrounding changes:
versionstext mode shows Local synced to: when known; checkout gains a regression test that.tmp-*staging dirs do not linger; multipart upload drops an unused URL-query parameter onnewStreamingMultipartRequest; docs cover overwrite semantics,--verify, and symlink behavior. Large integration/unit test coverage for verify, display composition, downloads, and cross-fix scenarios.Reviewed by Cursor Bugbot for commit 68a420e. Configure here.