Repository navigation
test(sync-memory): 11 behavioral tests for SUTANDO_MEMORY_DIR override (#1079 companion) - #1194
bassilkhilo-ag2 wants to merge 1 commit into
Conversation
VasiliyRad
left a comment
VasiliyRad
left a comment
There was a problem hiding this comment.
The diff does not match the PR description at all.
Title/description claim: 11 behavioral tests for sync-memory SUTANDO_MEMORY_DIR override.
Actual files changed:
build_log.md— 114 lines of workspace state (unrelated, should not be committed)src/health-check.py— multi-pid source-path enrichment (different feature entirely)tests/health-check-bridge-multi-pid.test.py— 5 structural tests for the health-check change
There are zero sync-memory test files in this PR.
Two issues:
- Wrong content: The code and tests are for a health-check improvement, not sync-memory. Either the branch or the PR description is wrong — the title needs to match what's actually in the diff.
- build_log.md bundled again: This is workspace runtime state and should not be committed to the repo. Remove it before merging.
The health-check change itself looks reasonable (showing script paths per PID in multi-process warnings is a real usability improvement, and the 5 tests verify the structural properties correctly). But it needs its own correctly-titled PR without the build_log.md.
VasiliyRad
left a comment
VasiliyRad
left a comment
There was a problem hiding this comment.
Mislabeled PR — content does not match title.
Title says test(sync-memory): 11 behavioral tests for SUTANDO_MEMORY_DIR override (#1079 companion). The actual diff contains:
build_log.md(+114 lines, new file) — bassilkhilo pass-207 log dumpsrc/health-check.py(+14/-2) — multi-PID source-path warn (same change as #1196)tests/health-check-bridge-multi-pid.test.py(+88) — same test file as #1196
Zero sync-memory tests in this PR. Either the branch is rebased over the wrong base, or the title points at a different PR than the one that actually ships those 11 tests.
Please:
- Confirm what this PR is actually supposed to land
- If the title is accurate, force-push the real sync-memory tests onto this branch
- If the title is wrong, rename and the dup changes vs #1196 still need to be resolved (#1196 also has these exact files)
Requesting changes — the PR as-is would land the same conflict-prone bundle as #1196 under a different name.
sonichi#1079 companion) Pin the structural and behavioral properties of the three-branch MEMORY_DIR resolution added in PR sonichi#1079 so a future refactor can't silently regress the multi-checkout fix. Tests (11 total, 1 pass on main / 11 pass after sonichi#1079 merges): Source-inspection (3): - SUTANDO_MEMORY_DIR override block present - SUTANDO_PRIVATE_DIR deprecation path present - safe tilde expansion form used (${VAR/#\~/$HOME}, not eval) Behavioral (8): - absolute SUTANDO_MEMORY_DIR honored - tilde-prefixed SUTANDO_MEMORY_DIR expanded to $HOME - unset SUTANDO_MEMORY_DIR falls back to SCRIPT_PARENT heuristic - empty string also falls back ([ -n "" ] is false) - SUTANDO_PRIVATE_DIR compat path resolves MEMORY_DIR - SUTANDO_PRIVATE_DIR emits deprecation warning to stderr - SUTANDO_MEMORY_DIR beats SUTANDO_PRIVATE_DIR when both set - multi-checkout scenario: override wins over wrong SCRIPT_PARENT Approach: awk-extract the MEMORY_DIR resolution block + run in a subshell with a fake SCRIPT_PARENT, mirror of tests/sync-memory-migration.test.sh. SYNC_MEMORY_SCRIPT env var lets test suites point at a non-default script. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9dcac3b to
67d7845
Compare
|
Fixed: rebased onto origin/main and cherry-picked the sync-memory test commit (67d7845). Branch now has exactly 1 commit — the intended 11 behavioral tests for |
|
The branch has been cleaned up since your review. Current diff is a single commit touching only Could you take another look at the current state? The content now matches the title. Re-requesting review. |
Summary
Companion test PR for #1079 (sync-memory: honor explicit SUTANDO_MEMORY_DIR override).
The existing
tests/sync-memory-migration.test.shcovers only the legacy-path migration block. It does NOT test the new three-branch MEMORY_DIR resolution that #1079 adds. This PR fills that gap.What's tested (11 tests)
Source-inspection (3) — verify structural properties:
SUTANDO_MEMORY_DIRoverride block is present insync-memory.shSUTANDO_PRIVATE_DIRdeprecation path is present${VAR/#\~/$HOME}form (noteval)Behavioral (8) — run the block in a subshell, assert MEMORY_DIR value:
SUTANDO_MEMORY_DIRis honoredSUTANDO_MEMORY_DIR=~/fooexpands to$HOME/fooSUTANDO_MEMORY_DIRfalls back toSCRIPT_PARENTheuristic (unchanged behavior)SUTANDO_MEMORY_DIR=""also falls back (same as unset)SUTANDO_PRIVATE_DIRcompat path resolvesMEMORY_DIRcorrectlySUTANDO_PRIVATE_DIRemits deprecation warning to stderrSUTANDO_MEMORY_DIRtakes precedence overSUTANDO_PRIVATE_DIRwhen both setSCRIPT_PARENTpathResult on current main
1/11 pass — only the fallback-heuristic behavioral test passes, because that path was always there. All others fail with "(override block not yet merged — PR #1079)". This is expected and correct.
Result after #1079 merges
11/11 pass (verified via
SYNC_MEMORY_SCRIPTenv-override pointing at the PR branch's script).Merge order
Merge #1079 first, then this.
🤖 Generated with Claude Code