From 081960db44c47f75fa9e05aad0cbb56e22b338ea Mon Sep 17 00:00:00 2001 From: colombod Date: Thu, 16 Jul 2026 18:05:05 +0000 Subject: [PATCH] fix(upload): resolve symlinks in legacy workspace slug + count only non-blank read lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up fixes from the PR #75 review: 1. Symlink parity: the live CI hook stamps a session's workspace via config_resolver._slug_from_working_dir(), which resolves symlinks (Path(working_dir).resolve()) before slugifying. The legacy import's derive_workspace() delegated to the hook's abspath-only low-level slugifier instead, so a working_dir traversing a symlink would import data into a DIFFERENT workspace than the hook wrote — latent drift the 0/3051 corpus sweep never hit because none of those sessions crossed a symlink. Align _slugify_path() to resolve first (preserving the empty -string default-fallback case), update the 4 parity tests to the resolved oracle, and add a real-symlink regression test. 2. Reconciliation arithmetic: _count_lines() counted all physical lines including blanks, but the operator summary's `read` field is documented as a non-blank event-line count and the upload loop skips blanks — so `read` could exceed ingested+skipped+unmapped from trailing blank lines. Count only non-blank lines so the summary reconciles by construction. Verified: 342 passed (ground-truth sweep 3286 ws_diffs=0, 400 data-parity 0 defects); ruff + ruff format clean; pyright 0 errors. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- .../legacy_transform.py | 40 ++++----- .../uploader.py | 7 +- .../tests/test_cli.py | 71 +++++++++++++++ .../tests/test_legacy_transform.py | 90 ++++++++++++++----- .../tests/test_uploader.py | 43 +++++++++ 5 files changed, 207 insertions(+), 44 deletions(-) diff --git a/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/legacy_transform.py b/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/legacy_transform.py index f0519cf..9171938 100644 --- a/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/legacy_transform.py +++ b/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/legacy_transform.py @@ -177,27 +177,27 @@ def reassemble_event_data(legacy_record: dict[str, Any]) -> tuple[str, dict[str, def _slugify_path(path: str) -> str: """Convert a working_dir path to the CI workspace slug. - Delegates directly to the live CI hook's own slugifier, - :func:`amplifier_module_hook_context_intelligence.config_resolver._slugify_path`, - so migrated data lands in the *exact* same workspace the live hook writes, - for every input the hook itself can receive. Reusing the hook's function -- - rather than re-implementing or partially re-implementing it -- makes - divergence structurally impossible: there is a single source of truth for - the slug. - - This matters beyond plain POSIX paths: the hook's ``config_resolver`` - normalises the raw ``session.working_dir`` value (backslash -> hyphen, - drive-letter colon stripped, leading-dash guaranteed, empty input falls - back to the default project slug) around calling the lower-level - ``workspace_slug``. An earlier revision here called bare ``workspace_slug`` - directly and additionally rejected empty/relative/root input outright -- - that diverged from the hook on exactly the inputs its own normalisation - exists to handle (e.g. a Windows-origin ``working_dir``), silently - dropping those sessions instead of placing them where the hook would. - There is no separate "correct" derivation to invent here: matching the - hook byte-for-byte on every input *is* correct by definition. + Mirrors the live CI hook's *default-config* workspace derivation, + ``HookConfigResolver._slug_from_working_dir``, which resolves the working + directory (symlinks included) before slugifying: + ``_hook_slugify_path(str(Path(working_dir).resolve()))``. Matching that + branch -- rather than the bare, non-resolving + :func:`amplifier_module_hook_context_intelligence.config_resolver._slugify_path` + the hook uses internally -- is what makes migrated data land in the + *exact* same workspace the live hook writes: the hook always resolves + ``session.working_dir`` before deriving a slug from it, so a + ``working_dir`` that traverses a symlink must be resolved here too, or + the import lands in a different workspace than the hook wrote to. + + The empty-string case is preserved as a special case (rather than being + resolved) because ``Path("").resolve()`` returns the current working + directory, not "no path" -- resolving it would silently manufacture a + bogus slug from the CWD instead of falling through to the hook's + documented empty-input fallback (``_DEFAULT_PROJECT_SLUG``). """ - return _hook_slugify_path(path) + if not path: + return _hook_slugify_path(path) + return _hook_slugify_path(str(Path(path).resolve())) def derive_workspace(working_dir: str) -> str: diff --git a/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/uploader.py b/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/uploader.py index b2913d1..db7f6a3 100644 --- a/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/uploader.py +++ b/modules/tool-context-intelligence-upload/amplifier_module_tool_context_intelligence_upload/uploader.py @@ -99,11 +99,12 @@ def _workspace_from_path(session_dir: Path) -> str: def _count_lines(file_path: Path) -> int: - """Count the number of lines in *file_path*.""" + """Count the number of non-blank lines in *file_path*.""" count = 0 with file_path.open(encoding="utf-8") as fh: - for _ in fh: - count += 1 + for raw_line in fh: + if raw_line.strip(): + count += 1 return count diff --git a/modules/tool-context-intelligence-upload/tests/test_cli.py b/modules/tool-context-intelligence-upload/tests/test_cli.py index 66916d8..6e421c5 100644 --- a/modules/tool-context-intelligence-upload/tests/test_cli.py +++ b/modules/tool-context-intelligence-upload/tests/test_cli.py @@ -1192,6 +1192,77 @@ def test_main_reconciliation_read_is_independent_line_count(self, tmp_path, caps captured = capsys.readouterr() assert "6 read" in captured.err + def test_main_reconciliation_read_excludes_blank_lines(self, tmp_path, capsys): + """read must count non-blank event lines only, so the arithmetic + (read == ingested + skipped + unmapped) reconciles exactly even when + events.jsonl has interior and trailing blank lines. + + Prior to the fix, _count_lines counted every physical line + (including blanks), so `read` could exceed + ingested + skipped + unmapped purely from blank/trailing lines -- + defeating the operator-trust arithmetic this summary exists for. + """ + from amplifier_module_tool_context_intelligence_upload import cli as cli_mod + from amplifier_module_tool_context_intelligence_upload.logging_hook_format import ( + LegacyDiscovery, + ) + from amplifier_module_tool_context_intelligence_upload.uploader import UploadResult + + session_dir = tmp_path / "session1" + session_dir.mkdir() + # 5 non-blank event lines interleaved with blank/whitespace-only lines + # (interior AND trailing) -- these must NOT be counted as "read". + (session_dir / "events.jsonl").write_text( + '{"line": 0}\n\n{"line": 1}\n{"line": 2}\n \n{"line": 3}\n{"line": 4}\n\n\n', + encoding="utf-8", + ) + metadata = {"session_id": "s1", "format": "logging-hook", "workspace": "ws"} + fake_sessions = [(session_dir, metadata)] + fake_discovery = LegacyDiscovery( + sessions=fake_sessions, + candidates_seen=1, + live_skipped=0, + unresolved_workspace=0, + ) + # ingested + skipped + unmapped == 5, matching the 5 non-blank lines. + upload_result = UploadResult( + success=True, + sessions_uploaded=1, + events_uploaded=3, + events_skipped=1, + events_unmapped=1, + ) + + with ( + patch( + "sys.argv", + [ + "context-intelligence-upload", + "--path", + str(tmp_path), + "--server-url", + "http://localhost", + "--api-key", + "key", + "--format", + "logging-hook", + ], + ), + patch.object(cli_mod, "discover_legacy", return_value=fake_discovery), + patch.object(cli_mod, "run_upload", return_value=upload_result), + patch.object(cli_mod, "ProgressTracker"), + pytest.raises(SystemExit), + ): + cli_mod.main() + + captured = capsys.readouterr() + assert "reconciliation:" in captured.err + # read == ingested + skipped + unmapped == 3 + 1 + 1 == 5, exactly. + assert "5 read" in captured.err + assert "3 ingested" in captured.err + assert "1 skipped" in captured.err + assert "1 unmapped" in captured.err + def test_main_reconciliation_default_format_zero_live_sessions_skipped(self, tmp_path, capsys): """The default context-intelligence path has no discovery object, so live-sessions-skipped must be 0 without raising.""" diff --git a/modules/tool-context-intelligence-upload/tests/test_legacy_transform.py b/modules/tool-context-intelligence-upload/tests/test_legacy_transform.py index 09be946..1c1dd0b 100644 --- a/modules/tool-context-intelligence-upload/tests/test_legacy_transform.py +++ b/modules/tool-context-intelligence-upload/tests/test_legacy_transform.py @@ -7,6 +7,8 @@ from __future__ import annotations import json +import os +from pathlib import Path import pytest from amplifier_module_hook_context_intelligence.config_resolver import ( @@ -157,18 +159,30 @@ def test_schema_guard_passes_clean_baseline_silently(capsys: pytest.CaptureFixtu # --------------------------------------------------------------------------- -# _slugify_path: EXACT parity with the live CI hook's own slugifier, -# ``amplifier_module_hook_context_intelligence.config_resolver._slugify_path`` -# (which itself delegates to ``context_intelligence.reconstruct.discover. +# _slugify_path: parity with the live CI hook's *resolve-then-slugify* branch, +# ``HookConfigResolver._slug_from_working_dir``, which calls +# ``_hook_slugify_path(str(Path(working_dir).resolve()))`` +# (``amplifier_module_hook_context_intelligence.config_resolver._slugify_path`` +# itself delegates to ``context_intelligence.reconstruct.discover. # workspace_slug`` and then applies Windows normalisation + a leading-dash / -# empty-input fallback). The hook is the ORACLE: legacy_transform must produce -# byte-identical output to it for every input, including Windows-origin paths -# and empty input -- there is no separate "correct" derivation, only "matches -# the hook or doesn't". Ground-truth parity against real hook-written data is +# empty-input fallback). That resolve-then-slugify branch is the ORACLE: +# legacy_transform must produce the same output as it for every input -- +# including Windows-origin paths, empty input, and symlinked working_dirs -- +# there is no separate "correct" derivation, only "matches the hook's write-time +# branch or doesn't". Ground-truth parity against real hook-written data is # proven in test_ground_truth_parity.py. # --------------------------------------------------------------------------- +def _resolved_oracle(raw_path: str) -> str: + """Compute the hook's ``_slug_from_working_dir`` oracle for *raw_path*. + + Mirrors ``HookConfigResolver._slug_from_working_dir`` exactly: resolve + symlinks/relative components, then slugify via the hook's own slugifier. + """ + return _hook_slugify_path(str(Path(raw_path).resolve())) + + def test_slugify_absolute_posix_unchanged() -> None: assert _slugify_path("/Users/me/project") == "-Users-me-project" @@ -193,11 +207,12 @@ def test_slug_matches_hook_for_windows_path() -> None: and drive-letter colons; the old ``legacy_transform._slugify_path`` instead rejected any non-POSIX-absolute path outright, so migrated Windows sessions were silently skipped instead of landing where the hook - would put them. This asserts exact parity, using the hook function itself - as the oracle (never a hardcoded expected string). + would put them. This asserts parity against the resolve-then-slugify + oracle (the hook's actual write-time branch), using the hook function + itself as the oracle (never a hardcoded expected string). """ windows_path = "C:\\Users\\me\\project" - assert derive_workspace(windows_path) == _hook_slugify_path(windows_path) + assert derive_workspace(windows_path) == _resolved_oracle(windows_path) @pytest.mark.parametrize( @@ -210,29 +225,62 @@ def test_slug_matches_hook_for_windows_path() -> None: ], ) def test_slug_matches_hook_across_inputs(raw_path: str) -> None: - """derive_workspace must equal the hook's own slugifier for every input. + """derive_workspace must equal the resolve-then-slugify oracle for every input. - The hook function is the oracle -- expected values are never hardcoded, - only compared against ``config_resolver._slugify_path`` directly, so this - test cannot pass by coincidence and cannot drift from the hook. + The oracle is the hook's actual write-time branch + (``HookConfigResolver._slug_from_working_dir``) -- expected values are + never hardcoded, only computed via ``Path(x).resolve()`` + + ``config_resolver._slugify_path`` directly, so this test cannot pass by + coincidence and cannot drift from the hook. """ - assert derive_workspace(raw_path) == _hook_slugify_path(raw_path) + assert derive_workspace(raw_path) == _resolved_oracle(raw_path) def test_slugify_root_matches_hook() -> None: # The hook does NOT reject a root path -- workspace_slug("/") == "-", and # the hook returns it as-is (truthy, already dash-prefixed). No raise. - assert _slugify_path("/") == _hook_slugify_path("/") + # "/" resolves to itself, so the resolved oracle is unaffected here. + assert _slugify_path("/") == _resolved_oracle("/") def test_derive_workspace_empty_matches_hook_default() -> None: # The hook returns its default project slug for empty input rather than - # failing -- match that exactly instead of raising. + # failing -- match that exactly instead of raising. Empty input is the + # one case NOT run through Path(...).resolve() (see _slugify_path + # docstring), so the oracle here stays bare (unresolved). assert derive_workspace("") == _hook_slugify_path("") def test_derive_workspace_relative_matches_hook() -> None: - # The hook resolves relative paths via os.path.abspath (cwd-dependent) -- - # it does not reject them. Compare against the oracle rather than a - # hardcoded string so the test is correct regardless of cwd. - assert derive_workspace("me/project") == _hook_slugify_path("me/project") + # The hook resolves relative paths (Path(...).resolve(), cwd-dependent) + # before slugifying -- it does not reject them. Compare against the + # resolved oracle rather than a hardcoded string so the test is correct + # regardless of cwd. + assert derive_workspace("me/project") == _resolved_oracle("me/project") + + +def test_derive_workspace_resolves_symlinked_working_dir(tmp_path: Path) -> None: + """A working_dir that traverses a symlink must land in the SAME workspace + the live hook would write to. + + The hook's ``_slug_from_working_dir`` always resolves ``session.working_dir`` + (``Path(working_dir).resolve()``) before slugifying, so a working_dir that + is -- or traverses -- a symlink must resolve to its target here too. This + is the regression this fix exists to close: prior to the fix, + ``derive_workspace`` matched the hook's *bare* (non-resolving) slugifier, + so a symlinked working_dir would import into a different workspace than + the one the live hook actually wrote to. + """ + target_dir = tmp_path / "real_project" + target_dir.mkdir() + link_path = tmp_path / "linked_project" + try: + os.symlink(target_dir, link_path) + except (OSError, NotImplementedError): + pytest.skip("OS does not support creating symlinks in this environment") + + expected = _resolved_oracle(str(link_path)) + assert derive_workspace(str(link_path)) == expected + # Sanity: the expected value really is derived from the RESOLVED target, + # not the symlink path itself (proves the test would fail pre-fix). + assert expected == _hook_slugify_path(str(target_dir.resolve())) diff --git a/modules/tool-context-intelligence-upload/tests/test_uploader.py b/modules/tool-context-intelligence-upload/tests/test_uploader.py index c95eddc..427ebd0 100644 --- a/modules/tool-context-intelligence-upload/tests/test_uploader.py +++ b/modules/tool-context-intelligence-upload/tests/test_uploader.py @@ -12,6 +12,7 @@ from amplifier_module_tool_context_intelligence_upload.uploader import ( UploadResult, + _count_lines, run_upload, ) @@ -726,3 +727,45 @@ def test_500_error_message_includes_server_body(self, tmp_path: Path) -> None: assert result.error is not None assert "500" in result.error assert "Internal Server Error" in result.error + + +# --------------------------------------------------------------------------- +# TestCountLines +# --------------------------------------------------------------------------- + + +class TestCountLines: + """Tests for _count_lines — must count non-blank lines only. + + The upload loop (run_upload) skips blank/whitespace-only lines when + processing events.jsonl (``if not line: continue``), so _count_lines must + apply the identical blank-check or the reconciliation arithmetic + (read == ingested + skipped + unmapped) breaks whenever a file has blank + interior or trailing lines. + """ + + def test_counts_non_blank_lines_only(self, tmp_path: Path) -> None: + """Interior and trailing blank lines are excluded from the count.""" + events_file = tmp_path / "events.jsonl" + # 3 non-blank lines, with a blank line between two of them and two + # trailing blank lines (whitespace-only counts as blank too). + events_file.write_text( + "line-one\n\nline-two\nline-three\n\n \n", + encoding="utf-8", + ) + + assert _count_lines(events_file) == 3 + + def test_counts_zero_for_all_blank_file(self, tmp_path: Path) -> None: + """A file containing only blank/whitespace lines counts as zero.""" + events_file = tmp_path / "events.jsonl" + events_file.write_text("\n\n \n\t\n", encoding="utf-8") + + assert _count_lines(events_file) == 0 + + def test_counts_all_lines_when_none_are_blank(self, tmp_path: Path) -> None: + """A file with no blank lines counts every line (baseline, no regression).""" + events_file = tmp_path / "events.jsonl" + events_file.write_text("a\nb\nc\n", encoding="utf-8") + + assert _count_lines(events_file) == 3