test(loom): compare worker cwd by identity - #406
Conversation
WalkthroughПроверка рабочего каталога в production-readiness тесте теперь сравнивает файловую идентичность директорий вместо строковых путей. Добавлены JSON-свидетельства о macOS-кейсе, исправлении и кроссплатформенном результате. ChangesКаноническая идентичность CWD
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
Review exact head c0637aa. Please judge semantic correctness of filesystem identity vs path-string equality, cross-platform behavior, test strength, and whether any production regression is hidden. @coderabbitai review @codex review @gemini-code-assist review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 406 --repo thebtf/engram --json title,body,headRefOid,files
echo "---DIFF---"
gh pr diff 406 --repo thebtf/engramLength of output: 7526 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,100p' internal/handlers/loom/production_readiness_coverage_test.goLength of output: 3231
No production source files are modified, so the "no hidden production regression" claim holds up structurally. Semantic correctness (identity vs. string equality): Cross-platform behavior:
Test strength:
The only theoretical gap: if Summary: The change is a sound, minimal, test-only fix that correctly swaps path-string equality for filesystem-identity comparison using stdlib primitives, matches the documented Go semantics, and doesn't touch production code. I'd approve this as-is; the optional ✅ Action performedReview finished.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a macOS-specific test failure in TestCliWorker_ProductionReadinessStructuredArgsAndCWD where exact string comparison of the current working directory (CWD) failed due to path aliasing (e.g., /var vs /private/var). The assertion was updated to use os.SameFile to verify directory identity rather than path spelling. No review comments were provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request addresses a macOS-specific test failure in TestCliWorker_ProductionReadinessStructuredArgsAndCWD where exact string comparison of the working directory failed due to /var vs /private/var path resolution. The assertion has been updated to use os.SameFile to verify directory identity instead of string equality. Additionally, two specification/evidence JSON files have been added to document this behavior. There are no review comments, so no feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/handlers/loom/production_readiness_coverage_test.go (1)
83-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winНеобязательно: явно проверяйте абсолютность
state.CWD.Сейчас относительный путь перед
os.Statразрешался бы относительно процесса теста, что может скрыть будущую регрессию helper-процесса.os.Getwd()сейчас возвращает абсолютный путь, поэтому это не блокер, но проверка черезfilepath.IsAbsявно зафиксирует контракт теста.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/handlers/loom/production_readiness_coverage_test.go` around lines 83 - 87, Добавьте явную проверку filepath.IsAbs для state.CWD в тесте, до вызова os.Stat, чтобы зафиксировать требование абсолютного пути; существующие проверки requestedDir, reportedDir и os.SameFile сохраните без изменений.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/handlers/loom/production_readiness_coverage_test.go`:
- Around line 83-87: Добавьте явную проверку filepath.IsAbs для state.CWD в
тесте, до вызова os.Stat, чтобы зафиксировать требование абсолютного пути;
существующие проверки requestedDir, reportedDir и os.SameFile сохраните без
изменений.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d693f33d-4c3f-443c-b3ea-494d1560eaf7
📒 Files selected for processing (3)
.agent/specs/macos-cwd-canonical/evidence/MACOS-CWD-CANONICAL.red.json.agent/specs/macos-cwd-canonical/evidence/MACOS-CWD-CANONICAL.tdd.jsoninternal/handlers/loom/production_readiness_coverage_test.go
|
Exact-head CI classification for c0637aa: downloaded artifacts from run 29245630994 show go_test_exit=0 on macOS, Ubuntu, and Windows, with 3913 tests and 0 failed on each. The original macOS /var versus /private/var assertion failure is absent, so os.SameFile proves the requested and reported directories identify the same filesystem object on the real macOS runner. The three matrix jobs are red only because the pre-governance base still treats 429 expected DB skips as unexpected and applies DB-owned coverage thresholds to non-DB matrices; the release job is likewise base-equivalent and outside this three-file test/evidence diff. Combined acceptance remains required after #405 and #406 are synthesized. CodeRabbit's filepath.IsAbs suggestion is non-blocking and intentionally not added: the product contract under repair is directory identity across aliases, which SameFile directly checks; widening this test-only batch would not change shipped behavior. |
1 participant
Outcome
Repairs the macOS cross-platform false red from run 29241241316 without changing CLI worker production behavior. The test now validates that the requested and reported CWD are the same existing directory via os.Stat plus os.SameFile, rather than requiring a unique path spelling that Go explicitly does not guarantee.
Evidence
External sources: official Go os.SameFile and filepath docs via Context7 and Parallel. Tavily remains OAuth-blocked and was not replaced with model memory.
Production code is unchanged. Final acceptance requires the real macos-14 check on this exact head.
Summary by CodeRabbit
Исправления
/varи/private/var.Тесты