ticket: use base32 project record ids
This commit is contained in:
@@ -0,0 +1,50 @@
|
||||
---
|
||||
title: "Tests: make path fallback tests independent from process env"
|
||||
state: "closed"
|
||||
created_at: "2026-05-31T10:46:14Z"
|
||||
updated_at: "2026-05-31T10:54:49Z"
|
||||
---
|
||||
|
||||
## Background
|
||||
|
||||
The environment-variable cleanup removed test-only env surfaces such as `INSOMNIA_TEST_*` and the Pod runtime command env override. The next issue is that several tests still mutate process-global environment even when they only need to verify deterministic fallback/precedence logic.
|
||||
|
||||
The user clarified the intended direction:
|
||||
|
||||
- even env vars used by the real application do not need to be read from process env in most tests;
|
||||
- tests should verify fallback/precedence behavior with direct inputs;
|
||||
- do not introduce a `PathEnv`/`EnvSnapshot` abstraction or a shared test-support crate just to preserve process-env mutation;
|
||||
- write the fallback helpers directly and narrowly where needed.
|
||||
|
||||
This ticket starts with path resolution, where the behavior is centralized and the env mutation is avoidable.
|
||||
|
||||
## Requirements
|
||||
|
||||
- In `manifest::paths`, separate fallback/precedence logic from process env reads enough that fallback tests can call pure helpers with direct `Option<PathBuf>`-style inputs.
|
||||
- Prefer small per-key helpers over a general `PathEnv` struct/trait.
|
||||
- Example shape: `resolve_config_dir_from_parts(...)`, `resolve_data_dir_from_parts(...)`, `resolve_runtime_dir_from_parts(...)`, or equivalent private helpers.
|
||||
- Keep helper visibility private or `pub(crate)` only if needed by nearby tests.
|
||||
- Keep runtime behavior unchanged:
|
||||
- public path functions still read the same supported env vars in the same order;
|
||||
- empty env values keep the current unset-equivalent behavior;
|
||||
- resource lookup fallback behavior remains compatible with packaging/dev use.
|
||||
- Convert path fallback tests that currently use `std::env::set_var` / `remove_var` to direct helper tests where possible.
|
||||
- Do not add a new helper crate or broad env abstraction.
|
||||
- Do not remove credential env behavior in this ticket; that belongs to `manifest-profile-encrypted-secrets`.
|
||||
- Do not attempt to eliminate subprocess integration env setup if a spawned process still needs runtime isolation; report any remaining env mutation rather than forcing risky API changes.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Removing all env reads from production code.
|
||||
- Removing provider credential env support.
|
||||
- Redesigning runtime directory authority or Pod process startup.
|
||||
- Introducing test-support / EnvSnapshot / trait-based environment abstraction.
|
||||
- Changing documented path semantics.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `manifest::paths` fallback/precedence tests no longer mutate process-global environment just to test fallback order.
|
||||
- The fallback order documented in `docs/environment.md` still matches the code.
|
||||
- Any remaining `set_var` / `remove_var` in touched tests is either eliminated or explicitly justified in the implementation report.
|
||||
- No new environment variable surface is introduced.
|
||||
- `cargo fmt --check`, `cargo test -p manifest paths`, relevant affected tests, `cargo check -p manifest`, `./tickets.sh doctor`, and `git diff --check` pass.
|
||||
@@ -0,0 +1,21 @@
|
||||
Made `manifest::paths` fallback tests independent from process-global environment mutation.
|
||||
|
||||
Implementation:
|
||||
- Added narrow private per-key fallback helpers for config/data/runtime path resolution.
|
||||
- Public path functions still read the same env vars and pass them in the same precedence order.
|
||||
- Empty path env values still resolve as unset.
|
||||
- Fallback-order tests now pass direct `Option<PathBuf>` inputs instead of using `std::env::set_var` / `remove_var`.
|
||||
- No `PathEnv`, `EnvSnapshot`, env trait, test-support crate, or new env var surface was introduced.
|
||||
- Credential env behavior and Pod runtime/process behavior were not changed.
|
||||
|
||||
Review:
|
||||
- External reviewer `pure-path-fallback-reviewer-20260531` approved implementation commit `e232f54`.
|
||||
|
||||
Validation after merge:
|
||||
- `cargo fmt --check`
|
||||
- `cargo test -p manifest paths`
|
||||
- `cargo test -p manifest`
|
||||
- `cargo check -p manifest` (passed with unrelated existing `llm-worker` dead_code warning)
|
||||
- `./tickets.sh doctor`
|
||||
- `git diff --check`
|
||||
- `git grep -n -E 'set_var\(|remove_var\(' -- crates/manifest/src/paths.rs || true` produced no matches.
|
||||
@@ -0,0 +1,131 @@
|
||||
<!-- event: create author: tickets.sh at: 2026-05-31T10:46:14Z -->
|
||||
|
||||
## Created
|
||||
|
||||
Created by tickets.sh create.
|
||||
|
||||
---
|
||||
|
||||
<!-- event: plan author: hare at: 2026-05-31T10:46:51Z -->
|
||||
|
||||
## Plan
|
||||
|
||||
Preflight classification: implementation-ready.
|
||||
|
||||
Intent:
|
||||
- Remove unnecessary process-global environment mutation from path fallback tests by making the fallback/precedence logic directly testable with explicit inputs.
|
||||
|
||||
Requirements:
|
||||
- Start with `manifest::paths`, where path resolution is centralized and currently owns env fallback tests.
|
||||
- Use small per-key pure helpers with direct optional path inputs. Do not introduce a general `PathEnv`, `EnvSnapshot`, env trait, or test-support crate.
|
||||
- Keep public runtime behavior unchanged: the production wrappers still read the same env vars, ignore empty path env values as before, and resolve the same fallback order.
|
||||
- Keep docs/environment.md behavior aligned with code.
|
||||
|
||||
Current code map:
|
||||
- `crates/manifest/src/paths.rs`: core config/data/runtime/resource path resolution and env fallback tests using local `EnvGuard`.
|
||||
- `docs/environment.md`: documented fallback order for home/config_dir/data_dir/runtime_dir/resource_dir.
|
||||
- Other env mutations in `pod`/`pod-registry` are mostly subprocess/runtime-isolation integration setup and should not be force-fixed in this ticket.
|
||||
- Provider credential env mutation belongs to `manifest-profile-encrypted-secrets`, not this ticket.
|
||||
|
||||
Critical risks:
|
||||
- Accidentally changing path precedence while making tests pure.
|
||||
- Creating an over-broad abstraction that keeps env mutation around indirectly.
|
||||
- Treating spawned-process runtime isolation env setup as equivalent to pure fallback tests and breaking integration tests.
|
||||
|
||||
Intent packet for implementation:
|
||||
|
||||
Intent:
|
||||
- Make `manifest::paths` fallback tests pure and independent from process env mutation.
|
||||
|
||||
Requirements:
|
||||
- Extract narrow pure helper functions for fallback precedence.
|
||||
- Convert fallback-order tests to call those helpers directly.
|
||||
- Keep production path functions and documented behavior unchanged.
|
||||
- Do not introduce a new crate, `PathEnv` struct, env trait, or test-only env var.
|
||||
|
||||
Invariants:
|
||||
- Path authority remains in `manifest::paths`.
|
||||
- No new environment-variable surface.
|
||||
- No provider credential/env cleanup in this ticket.
|
||||
- No Pod runtime/process model changes.
|
||||
|
||||
Non-goals:
|
||||
- Eliminating all `set_var`/`remove_var` in the repository.
|
||||
- Reworking resource packaging lookup beyond preserving behavior.
|
||||
|
||||
Escalate if:
|
||||
- Removing env mutation requires changing public APIs or path semantics.
|
||||
- Resource lookup behavior is ambiguous enough to need a product decision.
|
||||
- Pod runtime integration tests appear to need broader runtime-dir API changes.
|
||||
|
||||
Validation:
|
||||
- `cargo fmt --check`
|
||||
- `cargo test -p manifest paths`
|
||||
- relevant additional manifest tests if names differ
|
||||
- `cargo check -p manifest`
|
||||
- `./tickets.sh doctor`
|
||||
- `git diff --check`
|
||||
- report remaining `set_var`/`remove_var` in touched areas and why they remain, if any.
|
||||
|
||||
|
||||
---
|
||||
|
||||
<!-- event: review author: hare at: 2026-05-31T10:54:18Z status: approve -->
|
||||
|
||||
## Review: approve
|
||||
|
||||
External reviewer: `pure-path-fallback-reviewer-20260531`
|
||||
Reviewed implementation commit: `e232f54` (`test: make path fallback tests pure`)
|
||||
Verdict: approve
|
||||
|
||||
Summary:
|
||||
- `manifest::paths` fallback/precedence logic was split into private pure helpers.
|
||||
- Tests now pass direct `Option<PathBuf>` inputs to verify fallback order rather than mutating process environment.
|
||||
- No `PathEnv`/`EnvSnapshot`/trait/test-support crate was introduced.
|
||||
- The change is limited to `crates/manifest/src/paths.rs`.
|
||||
|
||||
Requirements mapping:
|
||||
- `config_dir`, `data_dir`, and `runtime_dir` public wrappers still read the same env keys and pass them in the existing precedence order.
|
||||
- Empty env values still become `None` through the retained `env_path`/path conversion behavior.
|
||||
- `docs/environment.md` fallback order matches the code.
|
||||
- `git grep -n -E 'set_var\(|remove_var\(' -- crates/manifest/src/paths.rs` has no matches.
|
||||
- No credential env behavior, Pod runtime/process behavior, or new env var surface was changed.
|
||||
|
||||
Blockers: none.
|
||||
Non-blocking follow-ups: none.
|
||||
|
||||
Validation adequacy:
|
||||
- Coder validation covered fmt, focused/full manifest tests, manifest check, doctor, diff check, and set_var/remove_var grep.
|
||||
- Reviewer performed read-only diff/source/docs consistency review and grep/diff-check spot checks; no additional Cargo tests were rerun by reviewer.
|
||||
|
||||
|
||||
---
|
||||
|
||||
<!-- event: close author: hare at: 2026-05-31T10:54:49Z status: closed -->
|
||||
|
||||
## Closed
|
||||
|
||||
Made `manifest::paths` fallback tests independent from process-global environment mutation.
|
||||
|
||||
Implementation:
|
||||
- Added narrow private per-key fallback helpers for config/data/runtime path resolution.
|
||||
- Public path functions still read the same env vars and pass them in the same precedence order.
|
||||
- Empty path env values still resolve as unset.
|
||||
- Fallback-order tests now pass direct `Option<PathBuf>` inputs instead of using `std::env::set_var` / `remove_var`.
|
||||
- No `PathEnv`, `EnvSnapshot`, env trait, test-support crate, or new env var surface was introduced.
|
||||
- Credential env behavior and Pod runtime/process behavior were not changed.
|
||||
|
||||
Review:
|
||||
- External reviewer `pure-path-fallback-reviewer-20260531` approved implementation commit `e232f54`.
|
||||
|
||||
Validation after merge:
|
||||
- `cargo fmt --check`
|
||||
- `cargo test -p manifest paths`
|
||||
- `cargo test -p manifest`
|
||||
- `cargo check -p manifest` (passed with unrelated existing `llm-worker` dead_code warning)
|
||||
- `./tickets.sh doctor`
|
||||
- `git diff --check`
|
||||
- `git grep -n -E 'set_var\(|remove_var\(' -- crates/manifest/src/paths.rs || true` produced no matches.
|
||||
|
||||
|
||||
---
|
||||
Reference in New Issue
Block a user