Skip to content

fix(agents): keep Windows checkouts and rendered docs platform-independent - #655

Open
tiendungdev wants to merge 1 commit into
zzet:mainfrom
tiendungdev:fix/windows-checkout-and-doc-path-portability
Open

fix(agents): keep Windows checkouts and rendered docs platform-independent#655
tiendungdev wants to merge 1 commit into
zzet:mainfrom
tiendungdev:fix/windows-checkout-and-doc-path-portability

Conversation

@tiendungdev

Copy link
Copy Markdown
Contributor

First tranche of the Windows CI work for #652. Scope is deliberately small and touches no runtime behaviour outside the rendered-document path: it removes the failures that block reading the Windows suite at all.

What is wrong

1. The repository has no .gitattributes.

Git for Windows installs with core.autocrlf=true — the GitHub windows-latest runner included — so a Windows checkout rewrites every text file to CRLF. Two things break:

  • internal/agents/opencode/plugin/gortex.js and internal/agents/pi/extension/index.ts are go:embed'd and written verbatim into a user's project. A source build on Windows therefore ships CRLF assets. TestPluginFailsOpen already detects it — it splits pluginSource on "\n}\n", which stops matching.

  • gofmt -l flags every Go file in the tree. Control on an untouched file:

    $ file -b internal/agents/aider/adapter.go
    Unicode text, UTF-8 text, with CRLF line terminators
    $ gofmt -l internal/agents/aider/adapter.go
    internal/agents/aider/adapter.go          # unformatted
    
    # same file, re-checked-out under the new .gitattributes
    $ file -b internal/agents/aider/adapter.go
    Unicode text, UTF-8 text
    $ gofmt -l internal/agents/aider/adapter.go
                                              # clean
    

    So any formatting or lint gate on the Windows leg would fail wholesale, and a Windows contributor cannot separate real findings from the noise.

The committed blobs are already LF. git add --renormalize . reports no content change — only the files this PR edits appear — so this pins the checkout and rewrites nothing.

*.ps1 keeps CRLF; *.png / *.gz are marked binary.

2. GlobalPointerBody builds the @-include with filepath.Join.

On Windows that writes

@C:\Users\me\.gortex\instructions\active.md

into ~/.claude/CLAUDE.md. That line is document content, not a filesystem call, and every other path in the file is /-spelled. shellSafeHookBinary already normalises unconditionally for exactly this reason, with the rationale written in its doc comment.

UpsertMarkedBlock keys idempotency on the start/end markers (instructions.go:203, :216), not on the path string, so an existing install has its block replaced in place on the next run — no duplicate, no orphan.

I checked for a consumer that parses the line back: the only production caller is claudecode/adapter.go:268, and it only writes.

3. normalizeRender scrubs only the native spelling.

gortexBinaryPaths() collects native paths (os.Executable, exec.LookPath), and home / root arrive native too — but a rendered manifest carries /-spelled paths by design. On Windows the ReplaceAll never matches, so TestAgentsRenderGolden leaks a machine-specific absolute into the comparison:

-@$HOME/.gortex/instructions/active.md
+@D:/tmp/gobuild/gortex-render-home-3991093423/.gortex/instructions/active.md

and it drifts for any developer who has gortex installed on PATH. This is the same class the file's own canonicalManifestKey already guards against.

Verification

windows/amd64, go1.26.6, -count=1, measured against origin/main 699f351e:

package before after
internal/agents ok ok
internal/agents/opencode 1 0
cmd/gortex 19 18
internal/agents/claudecode 2 2 — both pre-existing (TestResolveHookCommand, TestEmitPluginBundle_HookHandlerExecBit)

Newly-broken set is empty. gofmt and go vet clean on the touched files.

Each fix was sabotage-verified separately:

  • revert (1) → TestPluginFailsOpen: callHook has no closing brace this test can find
  • revert (2) → claude-code golden drifts to @$HOME\.gortex\instructions\active.md
  • revert (3) → claude-code and hermes goldens drift to absolute paths

TestAgentsRenderGolden now passes on Windows with the full PATH — i.e. with gortex actually installed — which it did not before.

Two assertions changed, and why

TestGlobalPointerBody_ShapeAndSentinel and TestGlobalInstall_FatToSlimReplacement built their expected @-include with filepath.Join. That asserts the native mangling on Windows, so it passes there whether or not the renderer normalises. They now use path.Join / filepath.ToSlash. In install_diet_test.go both spellings are kept deliberately — the @-include assertion takes the slash form, the os.ReadFile next to it keeps the native one.

Limitation, stated up front

As in #646, none of this can fail on the linux/macos matrix: filepath.ToSlash is a no-op on POSIX, and the CRLF conversion never happens there. The windows-latest leg from #652 is the only place these bind. I have not added a selector step — after your note on #652 that narrow selectors can miss the regression they are meant to protect, that seems like the wrong direction.

On the rest of #652

I ran the full suite on origin/main 699f351e (windows/amd64, go1.26.6, no -race): 173 failing tests across 28 packages, of which 2 are my machine, not the repointernal/analysis TestMapGitDiff* and internal/releases leave a .git/ai/working_logs/ directory behind that a local git fork creates, and os.RemoveAll then fails. I will post the full classification on #652 with the buckets and the three questions that need your call before I can fix them.

…ndent

Three defects that only surface on a Windows checkout, found while
enumerating the failures for the windows-latest matrix leg (zzet#652).

1. No .gitattributes, so a Windows checkout rewrites every text file to
   CRLF (Git for Windows installs with core.autocrlf=true, the GitHub
   windows runner included). Two consequences:

   - `internal/agents/opencode/plugin/gortex.js` and
     `internal/agents/pi/extension/index.ts` are go:embed'd and written
     verbatim into a user's project, so a source build on Windows ships
     CRLF assets. TestPluginFailsOpen already catches this: it splits
     pluginSource on "\n}\n", which no longer matches.
   - `gofmt -l` flags every Go file in the tree, so a formatting gate on
     the Windows leg fails wholesale and a Windows developer cannot tell
     real findings from the noise.

   The committed blobs are already LF — `git add --renormalize .` reports
   no content change — so this pins the checkout and rewrites nothing.

2. GlobalPointerBody built the @-include with filepath.Join, embedding
   `@C:\Users\me\.gortex\instructions\active.md` into ~/.claude/CLAUDE.md.
   That line is document content, not a filesystem call, and every other
   path in the same file is '/'-spelled. shellSafeHookBinary already
   normalises for exactly this reason. UpsertMarkedBlock keys idempotency
   on the markers, not on the path, so an existing install has its block
   rewritten in place on the next run.

3. normalizeRender scrubbed only the native spelling of HOME, the repo
   root and the resolved gortex binary, but a rendered manifest carries
   '/'-spelled paths by design. On Windows the substitution missed, so
   TestAgentsRenderGolden leaked a machine-specific absolute into the
   comparison and drifted for any developer with gortex on PATH.

Verification (windows/amd64, go1.26.6, -count=1):

  internal/agents            ok            (was ok)
  internal/agents/opencode   1 -> 0 failures
  cmd/gortex                19 -> 18 failures
  internal/agents/claudecode 2 -> 2 failures (both pre-existing:
    TestResolveHookCommand, TestEmitPluginBundle_HookHandlerExecBit)

Newly-broken set is empty. Each fix was sabotage-verified on its own:
reverting (1) fails TestPluginFailsOpen, reverting (2) drifts the
claude-code golden, reverting (3) drifts claude-code and hermes.

The two updated assertions built their expected @-include with
filepath.Join, which asserts the native mangling on Windows and passes
there whether or not the renderer normalises; they now use path.Join /
filepath.ToSlash. As in zzet#646, none of this can fail on the linux/macos
matrix — filepath.ToSlash is a no-op on POSIX — so the Windows runner is
the only place these bind.

@zzet zzet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: two Windows-only regressions are not protected by hosted CI.

  1. P2 — Exercise the LF checkout guarantee on Windows (.gitattributes:14)

    The Windows job runs go test ./internal/agents, which does not recurse into internal/agents/opencode; it therefore never runs TestPluginFailsOpen, the regression detector for CRLF-embedded gortex.js. Removing this attribute would leave hosted checks green while Windows source builds could ship CRLF assets again. Please add the OpenCode package test under a CRLF-prone Windows checkout, or an equivalent explicit attribute/byte assertion.

  2. P2 — Run the render golden on Windows (internal/agents/render.go:357)

    This second replacement only changes behavior on Windows because filepath.ToSlash is a no-op in the Linux/macOS jobs. TestAgentsRenderGolden lives in cmd/gortex, while the Windows job tests only ./internal/agents, so reverting this line would leave hosted checks green. Please run go test -timeout=5m -count=1 -run "^TestAgentsRenderGolden$" ./cmd/gortex on windows-latest, or add a direct Windows-binding unit test.

The implementation otherwise looks correct; I found no security or dead-code defects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants