Skip to content

refactor(hooks/builtins): inline GetEnvironmentInfo + simplify package#2529

Merged
dgageot merged 4 commits intodocker:mainfrom
dgageot:board/refactor-getenvironmentinfo-into-hooks-b-4c804132
Apr 27, 2026
Merged

refactor(hooks/builtins): inline GetEnvironmentInfo + simplify package#2529
dgageot merged 4 commits intodocker:mainfrom
dgageot:board/refactor-getenvironmentinfo-into-hooks-b-4c804132

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 27, 2026

Move session.GetEnvironmentInfo (and its only filesystem helper, isGitRepo) out of pkg/session and into pkg/hooks/builtins, where the lone production caller lives. Then take a small simplification pass over the builtins package now that everything is colocated.

The function had a single production caller — add_environment_info.go in pkg/hooks/builtins — and a handful of unexported helpers (boolToYesNo, getOperatingSystem, getArchitecture, isGitRepo) that nothing else in pkg/session referenced. Inlining the whole graph drops a cross-package import and lets the builtin testsuite exercise the helpers directly as internal-package tests.

Commits

  1. refactor(hooks/builtins): inline GetEnvironmentInfo and isGitRepo
    Move the function + its helpers + the related tests from pkg/session to pkg/hooks/builtins. Privatize on the way in (no external callers). Tests move with the code as internal-package tests so they can keep exercising the helpers.

  2. refactor(hooks/builtins): simplify add_environment_info
    Drop the boolToYesNo helper (one call site → inline ternary). Collapse the three-arm getArchitecture switch (only amd64 was non-identity) into a one-line if. Rename helpers to read as nouns (environmentInfo / displayOS / displayArch). Drop TestBoolToYesNo (helper gone) and TestGetEnvironmentInfoIntegration (duplicated the table-driven test, only differing by using os.Getwd as the cwd, which isn't a meaningfully distinct code path). Env-block format and addEnvironmentInfo session_start contract are byte-identical.

  3. refactor(hooks/builtins): consolidate isGitRepo tests into a single table
    Four single-case tests (TestIsGitRepo, TestIsGitRepoParent, TestInvalidGitFile, TestIsNotGitRepo) collapse into one table-driven TestIsGitRepo with six explicit cases (positive, parent-dir walkup, .git-is-a-file, missing/empty paths, etc.). Coverage is identical; the table makes the contract — what counts as "a git repo" — visible at a glance.

  4. refactor(hooks/builtins): drop dead IsZero and inline turnStartContext
    AgentDefaults.IsZero() had no production callers; the empty-config contract it documented is already enforced (and tested) directly via ApplyAgentDefaults returning nil. turnStartContext was a one-line wrapper used by exactly two callers, while addEnvironmentInfo was already calling hooks.NewAdditionalContextOutput directly — inlining surfaces every builtin's target event (turn_start vs session_start) at the call site rather than hiding it behind a same-package helper. Tighten the package doc accordingly.

Validation

  • go build ./...
  • mise test ✅ (97 packages OK, 0 failures)
  • mise lint ✅ (golangci-lint: 0 issues, custom lint: no offenses, go mod tidy clean)

Behavior preservation

  • Public API: only IsZero was removed, and it had no production callers.
  • The <env> block format injected at session_start is byte-identical (verified by the table-driven TestEnvironmentInfo).
  • Auto-injection of builtins from agent flags (AddDate / AddEnvironmentInfo / AddPromptFiles) is unchanged.

Assisted-By: docker-agent

dgageot added 4 commits April 27, 2026 14:26
session.GetEnvironmentInfo and session.isGitRepo had a single production caller (the add_environment_info builtin) and no callers elsewhere. Move both — along with their helpers (boolToYesNo, getOperatingSystem, getArchitecture) — into pkg/hooks/builtins where they're used, dropping the cross-package import. The function is privatized in the process; tests move with it as internal-package tests so they can keep exercising the helpers.

Assisted-By: docker-agent
- Drop the boolToYesNo helper: it had a single call site, replace with the idiomatic two-line ternary that yields it inline. - Replace the three-case getArchitecture switch (only amd64 was non-identity) with a one-line if. - Rename getEnvironmentInfo/getOperatingSystem/getArchitecture to environmentInfo/displayOS/displayArch so the names read as nouns/labels rather than getter calls. - Drop the now-redundant TestBoolToYesNo (the helper is gone) and TestGetEnvironmentInfoIntegration (it duplicated the table-driven test, only differing by using os.Getwd as the cwd, which is not a meaningfully distinct code path).

Behavior is preserved: the env block format, the public AddEnvironmentInfo registered name, and the addEnvironmentInfo session_start contract are all unchanged.

Assisted-By: docker-agent
…able

Four single-case tests collapsed into one table-driven TestIsGitRepo with six cases. Coverage is identical (positive parent-dir, .git-is-a-file, missing/empty paths, etc.) and the table layout makes the contract — what counts as 'a git repo' — visible at a glance.

Assisted-By: docker-agent
- AgentDefaults.IsZero() had no production callers; the empty-config contract it documented is already enforced (and tested) directly via ApplyAgentDefaults returning nil. Remove the method and the matching test assertion.

- turnStartContext was a one-line wrapper used by exactly two callers, while addEnvironmentInfo was already calling hooks.NewAdditionalContextOutput directly. Inline it in add_date and add_prompt_files so every builtin makes its target event (turn_start vs session_start) visible at the call site rather than hiding it behind a same-package helper.

- Tighten the package doc accordingly.

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 27, 2026 12:43
@dgageot dgageot merged commit b551afc into docker:main Apr 27, 2026
9 checks passed
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