test(winds): add deterministic T059 negative fixtures - #41
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded a test-only module with isolated fixtures and negative tests for workspace validation, Git cloning, shell and terminal failures, terminal-history retention, and environment-manifest inventory. ChangesNegative behavior tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
TheHalfMoon
force-pushed
the
test/003-t059-negative-fixtures
branch
from
August 17, 2026 14:53
3c2c551 to
9de6ffb
Compare
TheHalfMoon
force-pushed
the
test/003-t059-negative-fixtures
branch
from
August 17, 2026 14:56
cface77 to
d39549d
Compare
TheHalfMoon
commented
Aug 17, 2026
TheHalfMoon
left a comment
TheHalfMoon
left a comment
Owner
Author
There was a problem hiding this comment.
T059 exact-head correctness / safety / authority review — PASS
Reviewed exact head d39549d3190a4e069d3313df2563f86ba999b78b against canonical base ef985b11f67241ae45c623c89ccd03eb18441a0f and active Spec 003 / T059 only.
PASS rationale:
- the final diff is tests-only: one dedicated T059 acceptance module plus
#[cfg(test)]wiring; no production runtime or verification-authority behavior changes; - missing, non-Git, bare, invalid-symlink, and broken-symlink workspace fixtures fail before persistent workspace registration;
- credential-bearing clone coverage uses a local
file://remote, requires successful cloning, and checks both returned and SQLite-persisted remote identity for user-info removal without external network dependence; - clone failure remains explicit and does not fabricate workspace registration;
- disappearing shell executable is revalidated at launch time rather than trusting discovery-time identity;
- the unavailable-interpreter fixture reflects the actual PTY backend boundary: launch validation succeeds, PTY/session creation may succeed, but the child exec/start failure must become an observed non-zero final exit, must stay final on repeated observation, and must not execute the script body; no false-success or falsely-live claim is accepted;
- the separate immediate-exit fixture proves a normal explicit exit remains a single stable final result;
- the large-output fixture consumes 1 MiB while retaining only the configured 4 KiB transcript and proving total transcript+manifest storage remains inside the configured 64 KiB quota with truthful complete/truncated metadata;
- manifest inventory proves
.envrc,.mise.toml, and devcontainer contents are not executed or serialized as secret values; - temp cleanup is ownership-scoped to a canonical system-temp descendant with a Winds-owned prefix;
- Windows all-target compile/Clippy and the existing ConPTY/WSL/ledger/command suites remain green; Unix-only fixtures are explicitly cfg-gated;
- no T060 race/fault injection, stale-PID simulation, SQLite fault injection, shell-marker spoofing, daemon/protocol/Fleet, or new security claim is introduced.
Deterministic exact-head evidence observed: quality #370 PASS, windows-terminal #138 PASS after an infrastructure-only initial codeload 503/429 retry on the unchanged head, and release-candidate #206 PASS including SC-001 100-cycle soak plus Linux/macOS release builds/artifacts.
No correctness, safety, authority, data-loss, or active-scope blocker found on this exact head.
TheHalfMoon
commented
Aug 17, 2026
TheHalfMoon
left a comment
TheHalfMoon
left a comment
Owner
Author
There was a problem hiding this comment.
T059 exact-head Ponytail v4.9.0 simplicity review — PASS
Reviewed exact head d39549d3190a4e069d3313df2563f86ba999b78b for unnecessary implementation, abstraction, dependency, and future-scope leakage.
PASS rationale:
- no production code path, dependency, schema, migration, workflow, CLI, service, protocol, plugin/provider, daemon, renderer, or runtime abstraction was added;
- the suite reuses the already-accepted workspace, clone, shell-profile, PTY, history, and inventory seams directly rather than creating a new test framework;
- one dedicated test module keeps T059 acceptance evidence coherent, while
git.rsadds only#[cfg(test)]wiring; - the credential clone fixture is fully local and reuses system Git instead of introducing a mock transport/server;
- the PTY start-failure fixture deliberately avoids a synthetic PTY allocator/fault-injection interface. It tests the real unavailable-interpreter start/exec boundary and accepts the backend's truthful immediate non-zero exit semantics rather than modifying production behavior to satisfy a test assumption;
- generic allocator failure injection, concurrency races, SQLite failure injection, stale-PID simulation, and shell-hook spoof infrastructure are correctly left to T060 where the spec actually requires those fault surfaces;
- existing tests are not duplicated wholesale: the T059 suite adds explicit cross-slice acceptance assertions, including persisted clone sanitization and bounded 1 MiB transcript behavior, while remaining tests-only;
- platform-specific imports/helpers are cfg-gated only where necessary for Windows all-target compilation.
No justified simplification removes meaningful T059 evidence. No over-engineering blocker found on this exact head.
TheHalfMoon
marked this pull request as ready for review
August 17, 2026 15:02
Owner
Author
|
@coderabbitai review Please perform a fresh review of exact head Focus on deterministic fixture correctness, false-positive/false-success assertions, test cleanup/data safety, cross-platform compile assumptions, credential sanitization verification, PTY start/exec-failure truth, bounded-output assertions, and accidental T060+ scope leakage. The diff is tests-only and must not be treated as authorization for production runtime changes. |
Owner
Author
|
/review Review exact head |
PR Summary by QodoAdd deterministic T059 negative-fixture acceptance tests
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can route each action level your way: inline, summary, both, or drop |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='d39549d3190a4e069d3313df2563f86ba999b78b'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
git status --short
git diff --name-status "${expected}^" "$expected"
git diff --stat "${expected}^" "$expected"
git diff --check "${expected}^" "$expected"
git diff --no-ext-diff --unified=80 "${expected}^" "$expected" -- src/t059_negative_tests.rs src/git.rs
printf '\n== Outline: T059 tests ==\n'
ast-grep outline src/t059_negative_tests.rs --items all --view expanded || true
printf '\n== Outline: git module ==\n'
ast-grep outline src/git.rs --items all --view expanded || true
printf '\n== Relevant declarations and test hooks ==\n'
rg -n -C 5 --glob '*.rs' \
'pub\s+(async\s+)?fn\s+(register|launch|start|exec|observe|transcript|remote)|fn\s+(register|launch|start|exec|observe|transcript|remote)|cfg\(test\)|t059_negative_tests' \
srcLength of output: 53999 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Package and target configuration =='
fd -a -t f '^(Cargo\.toml|rust-toolchain.*|AGENTS\.md)$' . -x sh -c 'echo "--- $1"; sed -n "1,260p" "$1"' sh {} \;
printf '%s\n' '== Terminal session lifecycle =='
sed -n '45,230p' src/terminal.rs
printf '%s\n' '== Terminal Unix backend and exit handling =='
rg -n -C 8 --glob '*.rs' \
'fn (wait|try_wait|start_command|spawn)|impl TerminalSession|final_exit|exec|openpty|forkpty|waitpid' \
src/terminal.rs src
printf '%s\n' '== Shell profile discovery and launch validation =='
sed -n '1,285p' src/shell_profiles.rs
printf '%s\n' '== Clone sanitization and registration order =='
sed -n '1,270p' src/workspace_clone.rs
printf '%s\n' '== Workspace registration validation =='
sed -n '1,260p' src/workspace.rs
printf '%s\n' '== History reader, persistence, and quota accounting =='
rg -n -C 10 --glob '*.rs' \
'struct SessionHistoryRecorder|fn (wrap_output_reader|persist|local_bounded|history_logical_bytes|prune_for_write)|transcript_(observed|retained|capture_complete|truncated)|TOTAL|quota' \
src/command/history.rs
printf '%s\n' '== Environment inventory behavior =='
sed -n '1,235p' src/workspace_inventory.rsLength of output: 105836 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Manifest and target configuration files =='
fd -a -t f -g 'Cargo.toml' -g 'rust-toolchain*' -g 'AGENTS.md' . \
-x sh -c 'echo "--- $1"; cat "$1"' sh {} \;
printf '%s\n' '== Exact new-test lines =='
nl -ba src/t059_negative_tests.rs | sed -n '1,460p'
printf '%s\n' '== Exact terminal session methods =='
nl -ba src/terminal.rs | sed -n '45,190p'
rg -n -A 70 -B 15 'pub fn wait\(|pub fn try_wait\(|fn wait\(|fn try_wait\(' src/terminal.rs
printf '%s\n' '== Platform-specific terminal backend boundaries =='
rg -n -A 100 -B 15 \
'cfg\(unix\)|cfg\(windows\)|fn start_command|spawn|exec|forkpty|openpty|waitpid' \
src/terminal.rs
printf '%s\n' '== New-test cfg/import consistency =='
rg -n '^\s*#\[cfg|^use |TerminalSession|SessionHistory|Connection|symlink|initialize_repo|create_executable' \
src/t059_negative_tests.rs
printf '%s\n' '== Test module compilation topology =='
rg -n -C 3 'mod git|mod command|mod store|mod domain|mod execution|cfg\(test\)' src/lib.rs src/main.rs 2>/dev/null || true
printf '%s\n' '== Diff scope excludes production and T060 fault seams =='
git diff --no-ext-diff --name-only d39549d3190a4e069d3313df2563f86ba999b78b^ d39549d3190a4e069d3313df2563f86ba999b78b
git diff --no-ext-diff d39549d3190a4e069d3313df2563f86ba999b78b^ d39549d3190a4e069d3313df2563f86ba999b78b \
| rg -n -i 'fault.inject|openpty|allocator|resize|stale.pid|reuse|ownership.loss|race|daemon|ipc|protocol|plugin|mcp|a2a|acp|sandbox' || trueLength of output: 2604 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Configuration files =='
for file in Cargo.toml rust-toolchain rust-toolchain.toml AGENTS.md; do
if [ -f "$file" ]; then
printf '%s\n' "--- $file"
sed -n '1,320p' "$file"
fi
done
printf '%s\n' '== Exact T059 test source with line numbers =='
awk '{printf "%6d %s\n", NR, $0}' src/t059_negative_tests.rs
printf '%s\n' '== Terminal session lifecycle with line numbers =='
awk 'NR >= 45 && NR <= 240 {printf "%6d %s\n", NR, $0}' src/terminal.rs
printf '%s\n' '== wait / try_wait definitions =='
rg -n -A 80 -B 12 'pub fn (wait|try_wait)\(' src/terminal.rs
printf '%s\n' '== Unix and Windows terminal backend sections =='
rg -n -A 130 -B 10 \
'#\[cfg\((unix|windows)\)\]|fn start_command|forkpty|openpty|waitpid|exec' \
src/terminal.rs
printf '%s\n' '== Root module topology =='
sed -n '1,240p' src/main.rs
printf '%s\n' '== New test cfg markers and platform-sensitive symbols =='
rg -n -C 2 \
'#\[cfg|TerminalSession|SessionHistory|Connection|symlink|initialize_repo|create_executable' \
src/t059_negative_tests.rs
printf '%s\n' '== Exact diff scope and T060 keyword scan =='
git diff --no-ext-diff --name-only d39549d3190a4e069d3313df2563f86ba999b78b^ d39549d3190a4e069d3313df2563f86ba999b78b
git diff --no-ext-diff d39549d3190a4e069d3313df2563f86ba999b78b^ d39549d3190a4e069d3313df2563f86ba999b78b \
| rg -n -i 'fault.inject|allocator|resize|stale.pid|pid.reuse|ownership.loss|race|daemon|ipc|protocol|plugin|mcp|a2a|acp|sandbox' || trueLength of output: 68655
I found no blocking findings in the T059-only diff.
I used the repository workflow guidance that requires deterministic checks and a correctness/safety review. The sandbox did not execute repository code, so this result is a static review. The supplied exact-head CI evidence remains necessary for runtime validation. 🧠 Learnings used
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spec 003 / T059
Adds the deterministic negative-fixture acceptance suite required by T059 on top of canonical main:
ef985b11f67241ae45c623c89ccd03eb18441a0fFinal exact candidate head:
d39549d3190a4e069d3313df2563f86ba999b78bScope
Tests only. The final tree changes exactly:
src/t059_negative_tests.rs— dedicated T059 negative acceptance suitesrc/git.rs— three-line#[cfg(test)]module wiring onlyNo production runtime behavior, dependency, migration, workflow, schema, CLI, protocol, daemon, persistent-session, plugin/provider, MCP/ACP/A2A, Agent Fleet, Herdr/Pi, sandbox, or verification-authority behavior changes.
T059 fixture map
The suite explicitly exercises:
file://clone proving returned and SQLite-persisted remote identity strips user-info credentials without external network access;.envrc,.mise.toml, and devcontainer manifest inventory proving no auto-execution and no manifest secret-value serialization.Boundary to T060
This PR does not start T060. It does not introduce input/resize/exit races, SQLite fault injection, stale-PID/reuse fixtures, shell-hook spoofing, ownership-loss injection, or a generic fault-injection seam. A synthetic PTY allocator merely to force
openptyfailure is intentionally not introduced; the real child start/exec failure boundary is tested through the accepted backend semantics instead.Exact-head acceptance evidence
On
d39549d3190a4e069d3313df2563f86ba999b78b:windows-latest; attempt 1 failed before checkout/project execution because GitHub codeload returned HTTP 503/429 while downloading the pinned Rust action, and only that failed job was rerun successfully on the unchanged exact head;Qodo
/reviewwas additionally requested and acknowledged with an eyes reaction, but is not counted as acceptance evidence because no completed persistent result is currently available through the connector.T059 remains unchecked in
tasks.mduntil this accepted PR is merged with exact-head/unchanged-main guards and post-merge evidence is proven. T060+ remains not started.Summary by CodeRabbit