Add reusable Drive9 agent harness#427
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a repository-local, typed Go agent harness (phase‑1): design docs and README, YAML case suites with validation, CLI entrypoint, mount/process helpers, path safety checks, runner orchestration and workload implementations, recorder/report generation, JSON schemas, repro scripts, and unit tests. ChangesDrive9 Agent Harness v2 Implementation
Sequence Diagram(s) sequenceDiagram
participant User
participant CLI as drive9-agent-harness
participant Runner
participant Drive9 as drive9 binary
participant Recorder
participant Server
User->>CLI: run --suite regression
CLI->>Runner: parsed Config + command
Runner->>Drive9: start mount / run workloads
Drive9->>Runner: ready / command outputs
Runner->>Recorder: write events/failures/metrics
Runner->>Server: provision / check status (HTTP)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4f4ac4a9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| case writerErr = <-writerDone: | ||
| default: | ||
| writerErr = errors.New("writer interrupted before acknowledged completion") | ||
| } |
There was a problem hiding this comment.
If the write goroutine finishes before kill_after, the first select already consumes writerDone, but the second non-blocking receive treats the channel as empty and fabricates "writer interrupted before acknowledged completion". On fast filesystems this misclassifies a successful write as inconclusive, producing false failures for kill_during_write cases and obscuring real recovery behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e6bb4d0. runKillDuringWrite now preserves the writer result when the first select receives writerDone, and only restarts the mount when the kill path actually ran.
| ended := now() | ||
| var durationMS int64 | ||
| if manifest.StartedAt != "" { | ||
| if startedAt, err := time.Parse(time.RFC3339Nano, manifest.StartedAt); err == nil { | ||
| if endedAt, err := time.Parse(time.RFC3339Nano, ended); err == nil { | ||
| durationMS = endedAt.Sub(startedAt).Milliseconds() |
There was a problem hiding this comment.
Generate always sets ended_at to the current time, so running drive9-agent-harness report --run-dir ... hours or days later inflates duration_ms and rewrites the run as if it just ended. This makes historical timing data inaccurate and can corrupt trend comparisons built from regenerated artifacts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e6bb4d0. Generate now reads the persisted run_end event timestamp from events.jsonl and uses that for ended_at and duration_ms during report regeneration.
| if result.ExitCode != want { | ||
| recordOracleFailure(rec, c, "command_exit", result.ExitCode, want) |
There was a problem hiding this comment.
The doctor_fuse workload defines allow_nonzero_when_no_allow_other, but runDoctor ignores it and unconditionally fails whenever exit_code != expect_exit. In environments where drive9 doctor fuse returns nonzero solely because user_allow_other is missing, cases configured to tolerate that condition will still be marked as failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e6bb4d0. runDoctor now honors allow_nonzero_when_no_allow_other when the only failing doctor check is /etc/fuse.conf user_allow_other, with targeted test coverage.
| if cfg.APIKey != "" { | ||
| if err := checkStatus(ctx, cfg.Server, cfg.APIKey); err != nil { | ||
| return err |
There was a problem hiding this comment.
Preflight currently validates any provided API key even when --provision is set, so a stale DRIVE9_API_KEY causes preflight to fail although run --provision would successfully create a fresh tenant. This blocks the intended recovery path in environments where users keep expired keys in their shell profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e6bb4d0. Preflight now skips stale API-key status validation when --provision is set, and test coverage verifies that path.
| if result := mountproc.Run(ctx, "git-config-name", mountproc.Env{}, repoPath, git, "config", "user.name", userName); result.ExitCode != 0 { | ||
| recordResult(rec, c.ID, result) | ||
| recordOracleFailure(rec, c, "git_config", result.Stderr, "exit 0") |
There was a problem hiding this comment.
git_timeout is only used for fixture resolution and git clone; subsequent git config/add/commit/status calls run on the broader case context. If repository operations hang on a problematic mount, the workload can exceed the configured git-specific timeout and delay failure classification unexpectedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e6bb4d0. The git workflow now uses the git-specific context for config, add, commit, status, and commit-count commands, not just clone/fixture setup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
e2e/agent-harness/schemas/manifest.schema.json (1)
58-60: ⚡ Quick winType
casesentries instead of leaving them open.Line 58 declares
casesas a bare array with noitemsschema. That allows arbitrary shapes and undermines manifest schema value for tooling and report consumers.🤖 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 `@e2e/agent-harness/schemas/manifest.schema.json` around lines 58 - 60, The manifest's "cases" currently allows arbitrary entries; update the "cases" schema to specify "items" so each entry is typed (e.g., set "items": { "type": "object", "properties": { ... }, "required": [...], "additionalProperties": false } ), define the expected properties for a case (at minimum include the canonical identifiers/fields your tooling expects such as name/id and any payload/steps fields), mark required keys you need, and set additionalProperties to false to prevent unknown shapes; modify the "cases" schema accordingly in manifest.schema.json so consumers get a strict, documented structure.e2e/agent-harness/internal/casefile/casefile.go (1)
4-14: ⚡ Quick winAdd blank line between stdlib and third-party imports.
Imports should be grouped in three blocks separated by blank lines: stdlib, third-party, internal. Add a blank line before
gopkg.in/yaml.v3. As per coding guidelines, "Group imports in three blocks separated by blank lines: stdlib, third-party, internal".♻️ Proposed fix
import ( "errors" "fmt" "os" "path" "path/filepath" "strings" "time" + "gopkg.in/yaml.v3" )🤖 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 `@e2e/agent-harness/internal/casefile/casefile.go` around lines 4 - 14, The import block in casefile.go mixes stdlib and third-party imports; add a blank line between the stdlib imports (errors, fmt, os, path, filepath, strings, time) and the third-party import gopkg.in/yaml.v3 so imports are grouped into stdlib, third-party, internal sections; update the top import block (look for the line containing "gopkg.in/yaml.v3") to insert a blank line above it.e2e/agent-harness/internal/runner/runner_test.go (1)
87-109: ⚡ Quick winStrengthen error assertion to verify the specific failure cause.
Lines 106-108 only check that an error occurred, but don't verify that it's specifically a mountpoint safety error. The test would pass even if GC failed for an unrelated reason (I/O error, JSON parsing failure, etc.), weakening the test's value.
♻️ Proposed enhancement
If the runner package provides a sentinel error for safety violations:
err := GC(context.Background(), GCConfig{RunDir: runDir, ConfirmDelete: true, SuccessfulOnly: true}) - if err == nil { - t.Fatal("expected unsafe mountpoint rejection") + if !errors.Is(err, runner.ErrUnsafeMountpoint) { + t.Fatalf("err = %v, want ErrUnsafeMountpoint", err) }Or verify the error message contains expected text:
err := GC(context.Background(), GCConfig{RunDir: runDir, ConfirmDelete: true, SuccessfulOnly: true}) if err == nil { t.Fatal("expected unsafe mountpoint rejection") + } + if !strings.Contains(err.Error(), "unsafe") && !strings.Contains(err.Error(), "mountpoint") { + t.Fatalf("err = %v, want mountpoint safety error", err) }🤖 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 `@e2e/agent-harness/internal/runner/runner_test.go` around lines 87 - 109, Update TestGCRejectsUnsafeMountpoint to assert the failure is the specific mountpoint-safety error rather than any error: after calling GC(ctx, GCConfig{...}) use errors.Is(err, runner.ErrUnsafeMountpoint) (or the package-level sentinel error provided by the runner package) to verify the exact cause; if no sentinel exists, assert the error string contains the expected phrase (e.g., "unsafe mountpoint" or similar) using strings.Contains(err.Error(), "...") so the test fails only for unrelated errors (referencing TestGCRejectsUnsafeMountpoint, GC, GCConfig, and runner.ErrUnsafeMountpoint).
🤖 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.
Inline comments:
In `@docs/design/drive9-agent-harness-v2-plan.md`:
- Line 273: The doc and validator disagree about allowed sync_mode values: the
design doc lists only `strict`, `interactive`, or omitted, but the sync_mode
validator in ValidateCaseFile currently accepts `auto`; fix by either (A) adding
`auto` to the design doc with a one-line definition of its semantics and
examples, or (B) removing `auto` from the validator so it only permits `strict`,
`interactive`, or omitted; update the sync_mode validation logic in the
validator (ValidateCaseFile / sync_mode check) and any related tests or examples
to match the chosen option.
In `@e2e/agent-harness/cmd/drive9-agent-harness/main.go`:
- Around line 4-12: The import block in main.go merges stdlib and internal
imports; split them into separate groups by inserting a blank line between the
stdlib imports (context, flag, fmt, os, strings) and the internal import
(github.com/mem9-ai/dat9/e2e/agent-harness/internal/runner) so imports follow
the three-group convention (stdlib, third-party, internal) — update the import
statement surrounding the runner package accordingly.
In `@e2e/agent-harness/internal/mountproc/mountproc.go`:
- Around line 97-108: WaitMounted currently ignores errors returned by
check(mountpoint) and only returns ErrMountTimeout on context cancellation;
update WaitMounted to record the last non-nil error (e.g., lastErr) whenever
check returns err, continue polling as before, and when ctx.Done() is hit return
a wrapped error that includes ErrMountTimeout, the mountpoint, and the last
check error (if any) instead of dropping it so callers can diagnose the real
failure; reference the WaitMounted function and the check/mounted/err variables
and replace the ctx.Done() return branch to wrap and return lastErr when
present.
In `@e2e/agent-harness/internal/report/report.go`:
- Around line 302-310: The readFailures function uses bufio.NewScanner which can
overflow on large JSONL entries; modify readFailures to either (A) create the
scanner, call scanner.Buffer(make([]byte, 0, initialSize), maxTokenSize) with a
suitably large maxTokenSize constant before scanning, and then proceed to
json.Unmarshal each scanner.Bytes() as before, or (B) replace the scanner loop
with a json.Decoder on the file and iterate decoding into a Failure value
(decoder.Decode(&failure)) to robustly handle arbitrarily large JSON objects;
update references to scanner usage in readFailures accordingly.
In `@e2e/agent-harness/internal/runner/runner_test.go`:
- Around line 12-17: Define a package-level sentinel error
ErrConfirmDeleteRequired (errors.New("gc requires --confirm-delete")) in
runner.go, update the GC function to return that sentinel instead of a raw
string error, and change the test TestGCRequiresConfirmDelete to use
errors.Is(err, ErrConfirmDeleteRequired) (rather than comparing err.Error())
when checking the result of GC(context.Background(), GCConfig{RunDir:
t.TempDir()}); this matches the existing pattern used for ErrGateFailed.
In `@e2e/agent-harness/internal/runner/runner.go`:
- Around line 1417-1419: Network HTTP calls currently use
http.DefaultClient.Do(req) (e.g., the resp, err := http.DefaultClient.Do(req)
calls) which can block indefinitely; replace these with a dedicated *http.Client
that has a reasonable Timeout set and still respects caller context (attach ctx
to the request with req = req.WithContext(ctx) or build the request with
context) and then call client.Do(req). Update all occurrences (the shown call
and the similar ones around the other reported spots) to use the new client
instance instead of http.DefaultClient so network calls have a timeout while
preserving context cancellation.
- Around line 734-735: The code currently discards errors from
f.WriteString(c.Workload.Mutation.Content) and f.Close(); change this to capture
and handle both errors (e.g., err := f.WriteString(...); if err != nil {
return/log/propagate the error } and similarly check the result of f.Close(),
using explicit ignore only with `_ = f.Close()` if you intentionally want to
ignore close errors). Ensure you update the caller path to propagate or log the
error so a failed write/close doesn't produce false pass/fail signals and
reference f.WriteString, f.Close, and c.Workload.Mutation.Content when making
the changes.
In `@e2e/agent-harness/internal/safety/safety.go`:
- Around line 67-69: The mountpoint check currently treats IsMounted(mp) errors
as success—change the logic in the function containing this snippet so any
non-nil error from IsMounted is returned (wrapped with context) instead of being
ignored; keep the existing behavior of returning ErrExistingMountpoint when
IsMounted returns (nil, true), but when IsMounted returns an error return a
fmt.Errorf with context (e.g., include mp) and the original error so callers can
detect mount-detection failures; update all references to IsMounted,
ErrExistingMountpoint and the mp variable in that block accordingly.
In `@e2e/agent-harness/schemas/case.schema.json`:
- Around line 14-137: The nested object schemas under the case item (workload,
each oracles[] item, and severity) currently allow extra properties; add
"additionalProperties": false to the "workload" object schema, to the "items"
object schema inside "oracles", and to the "severity" object schema so typos
like "worklod" or "sevverity" are rejected (top-level already enforces
additionalProperties: false); update the schemas that define "workload",
"oracles" -> "items", and "severity" to include this constraint.
In `@e2e/agent-harness/schemas/gating.schema.json`:
- Around line 31-45: The integer counter fields pass, fail,
known_bug_reproduced, known_bug_fixed_candidate, and non_gating in
gating.schema.json need a non-negative constraint; update each property's schema
to include "minimum: 0" (keeping "type": "integer") so these counters cannot be
negative and schema validation will reject negative values.
In `@e2e/agent-harness/schemas/summary.schema.json`:
- Around line 42-53: The cases.items schema currently only lists required fields
but lacks properties to validate them; update the cases.items object to include
a properties block defining: id as a string with a minLength (e.g., 1), suite as
an enum ["smoke","regression","stress","fault"], expected_outcome as an enum
["baseline_pass","bug_reproduced","fix_verified"], and status as an enum
["passed","failed"]; keep the existing required array and ensure each property
has the correct JSON Schema type/constraints so validation will enforce values
for id, suite, expected_outcome, and status.
---
Nitpick comments:
In `@e2e/agent-harness/internal/casefile/casefile.go`:
- Around line 4-14: The import block in casefile.go mixes stdlib and third-party
imports; add a blank line between the stdlib imports (errors, fmt, os, path,
filepath, strings, time) and the third-party import gopkg.in/yaml.v3 so imports
are grouped into stdlib, third-party, internal sections; update the top import
block (look for the line containing "gopkg.in/yaml.v3") to insert a blank line
above it.
In `@e2e/agent-harness/internal/runner/runner_test.go`:
- Around line 87-109: Update TestGCRejectsUnsafeMountpoint to assert the failure
is the specific mountpoint-safety error rather than any error: after calling
GC(ctx, GCConfig{...}) use errors.Is(err, runner.ErrUnsafeMountpoint) (or the
package-level sentinel error provided by the runner package) to verify the exact
cause; if no sentinel exists, assert the error string contains the expected
phrase (e.g., "unsafe mountpoint" or similar) using
strings.Contains(err.Error(), "...") so the test fails only for unrelated errors
(referencing TestGCRejectsUnsafeMountpoint, GC, GCConfig, and
runner.ErrUnsafeMountpoint).
In `@e2e/agent-harness/schemas/manifest.schema.json`:
- Around line 58-60: The manifest's "cases" currently allows arbitrary entries;
update the "cases" schema to specify "items" so each entry is typed (e.g., set
"items": { "type": "object", "properties": { ... }, "required": [...],
"additionalProperties": false } ), define the expected properties for a case (at
minimum include the canonical identifiers/fields your tooling expects such as
name/id and any payload/steps fields), mark required keys you need, and set
additionalProperties to false to prevent unknown shapes; modify the "cases"
schema accordingly in manifest.schema.json so consumers get a strict, documented
structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca88cda9-6288-4d3f-af62-829164a120f3
📒 Files selected for processing (28)
docs/design/drive9-agent-harness-v2-plan.mde2e/agent-harness/README.mde2e/agent-harness/cases/fault.yamle2e/agent-harness/cases/regression.yamle2e/agent-harness/cases/smoke.yamle2e/agent-harness/cases/stress.yamle2e/agent-harness/cmd/drive9-agent-harness/main.goe2e/agent-harness/internal/casefile/casefile.goe2e/agent-harness/internal/casefile/casefile_test.goe2e/agent-harness/internal/mountproc/mountproc.goe2e/agent-harness/internal/mountproc/proc_unix.goe2e/agent-harness/internal/mountproc/proc_windows.goe2e/agent-harness/internal/report/report.goe2e/agent-harness/internal/report/report_test.goe2e/agent-harness/internal/runner/runner.goe2e/agent-harness/internal/runner/runner_test.goe2e/agent-harness/internal/safety/safety.goe2e/agent-harness/internal/safety/safety_test.goe2e/agent-harness/repro/known-git-lock.she2e/agent-harness/repro/known-path-edge.she2e/agent-harness/schemas/case.schema.jsone2e/agent-harness/schemas/event.schema.jsone2e/agent-harness/schemas/failure.schema.jsone2e/agent-harness/schemas/gating.schema.jsone2e/agent-harness/schemas/manifest.schema.jsone2e/agent-harness/schemas/metric.schema.jsone2e/agent-harness/schemas/summary.schema.jsongo.mod
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
PR #427 — Add reusable Drive9 agent harness: REVISE
Reviewed: design doc, 4 suite YAMLs, 6 JSON schemas, cmd/main.go, casefile, mountproc, report, runner (1582 LoC), safety, repro scripts, README, go.mod. Head: dc0f1ac.
Design and Structure
The overall architecture is sound: typed YAML cases → runner with structured JSONL/JSON artifacts → machine-readable gating → regenerable reports. The design doc is thorough. Safety model is well-considered. The phase-1 / deferred boundary is clear.
Blockers
B1. ValidateRoot does not reject remote-root-base paths (non-filesystem paths)
`safety.ValidateRoot("remote-root-base", ...)` checks `strings.HasPrefix(p, "/")` — this accidentally works for remote roots like `/agent-adversarial-xxx`, but the function name and contract say "validate root" meaning a local filesystem root. If someone passes a relative remote root or one without a leading slash, `ValidateRoot` returns the right error, but the error message says "must be absolute" — which is a filesystem concept, not a remote-path concept. The function should either be split into `ValidateLocalRoot` and `ValidateRemoteRootBase`, or the error message should be parameterized. Currently the coupling is accidental — a future caller will misread the contract.
B2. runID() is not collision-safe for concurrent or rapid sequential runs
`runID()` returns `time.Now().UTC().Format("20060102T150405Z")` — second granularity. Two runs within the same second get the same run ID, same artifact directory, same remote root base. This will silently corrupt artifacts. Add a random suffix (e.g. 4 hex chars from crypto/rand) or use a monotonic counter.
B3. shouldRemoveMountpoint reads retErr via closure but retErr is always nil at call site
In `runCase`, the defer closure calls `shouldRemoveMountpoint(c, rec, retErr)`, but `retErr` is the named return value. At the point the defer captures `retErr`, it's still nil because the defer evaluates it lazily. However, the doctor_fuse path calls `shouldRemoveMountpoint(c, rec, retErr)` directly at line ~2778 where `retErr` is indeed nil (the function hasn't returned yet). This means doctor_fuse cleanup always thinks there was no error, so `retain_on_failure` is broken for doctor_fuse cases — mountpoints are always removed even on failure.
B4. path_matrix path validation rejects the design doc's own test path
The regression case `path-edge-strict` defines path `"space name unicode-测试 punct_!@%.txt"`. But `ValidateRelativeSlashPath` calls `path.Clean(p)` and checks `clean != p`. `path.Clean("space name unicode-测试 punct_!@%.txt")` returns the same string, so this specific path passes. However, paths containing consecutive spaces, trailing spaces, or trailing dots would be silently cleaned by `path.Clean`, causing the `clean != p` check to reject them — but these are exactly the edge cases the path matrix workload is supposed to test. The validation is over-restrictive for a path-edge regression suite whose entire purpose is testing unusual paths. Consider relaxing validation for `path_matrix` workloads specifically, or documenting why the restriction exists.
B5. provision() sends POST with nil body — server may reject
`http.NewRequestWithContext(ctx, http.MethodPost, .../v1/provision, nil)` sends a POST with no Content-Type and no body. Depending on the server implementation, this may be rejected. At minimum, send an empty JSON object body with `Content-Type: application/json`.
B6. Generate() double-counts cases that appear in both manifest.Cases and manifest.SelectedCases
`Generate` initializes `selected` from `manifest.Cases`, then iterates `manifest.SelectedCases` and adds entries that aren't already present. But in `Run()`, the manifest is written with both `Cases: caseSummaries(cases)` and `SelectedCases: caseIDs(cases)` — the same case IDs. When `Generate` then iterates `selected` to count pass/fail, it iterates the merged map, which is correct. But: if a case has failures, the code updates `selected[f.CaseID]` with `ExpectedOutcome` from the failure record, overwriting the suite field from the original `CaseSummary`. A case that fails with one oracle but passes another will get its `Suite` field zeroed out (because `Failure` doesn't carry `Suite`), causing `summary.Counts.BySuite[""]` to increment. The suite field must be preserved from the original manifest, not overwritten by failure iteration.
Non-blocking observations
-
Duplicate
readJSON/writeJSON— defined in bothreport/report.goandrunner/runner.go. Factor into a sharedinternal/jsonutilpackage. -
IsMountedon macOS — parsesmountcommand output withstrings.Contains(string(out), " on "+clean+" "). This is fragile — mount paths with spaces or parentheses in the mount options will false-match. Not a blocker for phase-1 local testing, but worth a TODO. -
open_fd_unmountfirst unmount timing — the test holds a file open forhold_open_duration(1s), then attempts unmount. But the write (f.WriteString("held open by harness\n")) happens before the hold delay. In write-back mode, the data may already be flushed by the time the unmount runs, making the "busy" path unreliable. Consider writing continuously during the hold period. -
kill_during_writegoroutine leak on context cancel — ifctxis cancelled before the timer fires and before the writer goroutine finishes, the goroutine continues writing to the (now possibly unmounted) FUSE path indefinitely. The writer should also select onctx.Done()— which it does viawriteSizedFile, butwriteSizedFileonly checks ctx between 1MB blocks. A 67MB write with 1MB blocks means up to 1MB of additional writes after cancellation. -
Missing
Workload.Remountdefault —remountPathschecksc.Workload.Remount != nil && !*c.Workload.Remountto skip remount. ButRemountis a*bool— when omitted from YAML, it's nil, so remount is always attempted. The design doc says "Optionalremount: true|false, defaulting totrue" — this matches the nil-means-true behavior, but it's implicit. Document the nil semantics or set an explicit default inapplyDefaults. -
path_matrixcontent derivation — content defaults toc.ID + ":" + p. For the path-edge case, this generates content likepath-edge-strict:space name unicode-测试 punct_!@%.txt. The content itself contains the problematic characters — which is fine for testing, but the SHA-256 oracle derivation depends on this exact template. If someone addscontent_templateto a case without updating oracles, the derived oracle silently uses different content. Worth a comment. -
No test for
FilterCaseswith unknown case IDs —FilterCasessilently drops unknown IDs. If someone typos--case path-edge-strct, they get zero cases and the "no selected cases" error doesn't tell them which ID was wrong.
Verdict: REVISE
Fix B1–B6 before merge. B2 (runID collision) and B3 (retain_on_failure broken for doctor_fuse) are correctness bugs. B6 (suite field zeroed) corrupts summary reports.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
PR #427 Review — Add reusable Drive9 agent harness
Overall this is a solid, well-structured harness with good design principles (typed case files, structured artifacts, safety validation, oracle-based verification). The design doc is thorough and the contract-first approach is the right call.
I have several issues to flag, ranging from potential data corruption to API contract mismatches. See inline comments.
Summary
Architecture & Design — Clean separation of concerns across casefile, safety, mountproc, report, and runner. The YAML case schema is well-typed and the oracle system is extensible.
Key Issues:
--sync-modeflag is deprecated — PR #431 replaced--sync-modewith--durability. The harness will break on latest main.shouldRemoveMountpointcaptures staleretErr— The defer closure readsretErrat return time but the variable is alwaysnilat the point where it's evaluated for doctor_fuse cases.Lstatwon't detect symlinks —os.Lstatreturns the link info, butinfo.Mode()&os.ModeSymlink != 0is always true for Lstat — the check logic is correct, but needs clarity.- Race in
runKillDuringWrite— Writer goroutine and kill timer can race; after kill, the secondselectonwriterDoneusesdefaultbranch which may miss a completed writer. appendJSONLnot safe for concurrent writers — Multiple goroutines could callrec.Event()/rec.Failure()concurrently without synchronization.- Missing
go.sumupdate —gopkg.in/yaml.v3promoted from indirect to direct but no go.sum diff shown.
Nits:
readJSONandwriteJSONare duplicated in bothreportandrunnerpackages.- Design doc is 691 lines — consider splitting deferred sections into a separate doc.
| import ( | ||
| "bytes" | ||
| "context" | ||
| "errors" |
There was a problem hiding this comment.
--sync-mode is deprecated after PR #431 merge — PR #431 replaced --sync-mode with --durability. This harness uses --sync-mode in StartMount() (line ~2006 in diff):
args = append(args, "--sync-mode", syncMode)This will break on current main. Need to map the case sync_mode values (strict, interactive, auto) to the new --durability flag values (auto, interactive, fsync, close-sync, write-sync), or keep using --sync-mode if backward compat is maintained. Verify against current CLI.
There was a problem hiding this comment.
Addressed in 53692d5. StartMount now detects whether the target drive9 mount --help exposes --durability or legacy --sync-mode; for current main it maps sync_mode values to durability (strict -> fsync, interactive -> interactive, auto -> auto).
| _ = rec.Event(report.Event{CaseID: c.ID, Type: "case_end"}) | ||
| if shouldRemoveMountpoint(c, rec, retErr) { | ||
| _ = os.Remove(mountpoint) | ||
| } |
There was a problem hiding this comment.
shouldRemoveMountpoint captures stale retErr for doctor_fuse
In the doctor_fuse branch (line ~2778 in diff):
if shouldRemoveMountpoint(c, rec, retErr) {
_ = os.Remove(mountpoint)
}retErr is the named return value, but at this point it's always nil because no error has been assigned yet — the function returns nil explicitly on line 2781. This means retain_on_failure cases will always clean up the mountpoint for doctor_fuse, even if the case recorded oracle failures.
The intent is probably shouldRemoveMountpoint(c, rec, nil) here (which is what happens), but the pattern is misleading. The caseHadFailures fallback in shouldRemoveMountpoint does check failures.jsonl, so this actually works correctly — but the retErr parameter is dead code for this branch. Consider documenting this or simplifying.
There was a problem hiding this comment.
Agree with the analysis. No behavior change needed here: the doctor branch relies on caseHadFailures inside shouldRemoveMountpoint, so retain_on_failure still retains when oracle failures were recorded. The retErr argument is redundant in this branch but not currently harmful.
| recordResult(rec, caseID, result) | ||
| return result | ||
| } | ||
|
|
There was a problem hiding this comment.
appendJSONL is not safe for concurrent use — runParallelWrites spawns goroutines that call recordOracleFailure → rec.Failure() → appendJSONL. Similarly, multiple concurrent oracle checks could call rec.Event(). The appendJSONL function opens the file with O_APPEND which provides atomic appends on most POSIX systems for writes < PIPE_BUF, but:
- The JSON marshal + write is not atomic — two goroutines could interleave partial lines
- On some filesystems,
O_APPENDatomicity isn't guaranteed for writes > 4KB
Add a mutex to Recorder or use a channel-based writer for thread safety.
There was a problem hiding this comment.
I do not think this is currently reachable from runParallelWrites: the goroutines only collect errors into a local slice under a mutex, and recordOracleFailure runs after wg.Wait. Cases/oracles are otherwise executed serially. If we later parallelize case execution or oracle recording, adding a Recorder mutex would be the right boundary.
There was a problem hiding this comment.
Follow-up addressed in fd39e97. Even though current runParallelWrites records failures after wg.Wait, Recorder now serializes artifact writes with a sync.Mutex, covering future concurrent Event, Failure, and Metric calls as well.
| return nil | ||
| } | ||
|
|
||
| func runKillDuringWrite(ctx context.Context, rec *report.Recorder, manifest *report.Manifest, env mountproc.Env, drive9Bin, caseRemote, mountpoint string, c casefile.Case, mount **mountproc.Mount) error { |
There was a problem hiding this comment.
Race in runKillDuringWrite — After the kill timer fires and mount is killed, the second select block:
select {
case writerErr = <-writerDone:
default:
writerErr = errors.New("writer interrupted before acknowledged completion")
}The default branch fires immediately if the writer goroutine hasn't returned yet. But the writer might complete (with an error from the killed mount) between the kill and this select. This is a race — you could miss the writer's actual error. Consider using a short timeout or always draining the channel:
select {
case writerErr = <-writerDone:
case <-time.After(2 * time.Second):
writerErr = errors.New("writer interrupted before acknowledged completion")
}There was a problem hiding this comment.
Valid. e6bb4d0 fixed the consumed-success path from the earlier review, but this remaining post-kill drain race is still worth tightening with a bounded wait for writerDone. Not addressed in the current commit.
There was a problem hiding this comment.
Addressed in 53692d5. After the kill path fires, runKillDuringWrite now uses a bounded wait for writerDone instead of an immediate default branch, while still respecting context cancellation.
| info, err := os.Lstat(mp) | ||
| if os.IsNotExist(err) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Symlink check is correct but misleading — os.Lstat returns the file info without following symlinks, so info.Mode()&os.ModeSymlink != 0 correctly detects symlinks. Good. However, the subsequent info.IsDir() check on line 4329 would pass for a symlink pointing to a directory if you used os.Stat instead — since you're using Lstat, IsDir() returns false for symlink-to-dir, which means the function correctly rejects symlinks before reaching the directory check.
This is fine, just noting for future readers that the order matters here.
There was a problem hiding this comment.
Agree. No code change needed; Lstat is intentional here so symlink-to-dir paths are rejected before any directory availability check.
| } | ||
|
|
||
| func runParallelWrites(ctx context.Context, rec *report.Recorder, mountpoint string, c casefile.Case) error { | ||
| start := time.Now() |
There was a problem hiding this comment.
i := i shadow is unnecessary with Go 1.22+ — If the project uses Go 1.22+, the loop variable capture change makes i := i redundant. Check go.mod for the Go version — if ≥ 1.22, this can be removed for clarity. (Not a bug, just noise.)
There was a problem hiding this comment.
Agree this is redundant on the repository’s Go version. I left it unchanged because it is harmless and consistent with the explicit capture style in this harness code; it can be cleaned up separately.
| } | ||
|
|
||
| func Run(ctx context.Context, cfg Config) (string, error) { | ||
| id := runID() |
There was a problem hiding this comment.
Manifest written after all cases, but cases mutate it during execution — manifest.GeneratedMountpoints, manifest.GeneratedRemoteRoots, and manifest.ProcessGroups are populated during runCase iterations. If the runner crashes mid-run, the manifest is never written and all run metadata is lost. Consider writing a partial manifest before the case loop and updating it after each case, or at minimum writing it in a defer.
There was a problem hiding this comment.
Valid resilience improvement. Not addressed in e6bb4d0; a follow-up should write an initial manifest before the loop and update it after each case, or defer a best-effort final manifest write.
There was a problem hiding this comment.
Addressed in 53692d5. The runner now writes an initial manifest before the case loop and refreshes it after each case, so generated remote roots, mountpoints, and process metadata are persisted incrementally.
| mountpoint_suffix: path-edge-strict | ||
| workload: | ||
| type: path_matrix | ||
| paths: |
There was a problem hiding this comment.
path_matrix validation vs actual path — The path "space name unicode-测试 punct_!@%.txt" will pass ValidateRelativeSlashPath since it has no /, .., or backslashes. But the % character is URL-significant and could cause issues with the drive9 CLI's fs cp / fs cat commands if they don't properly encode the remote path. The harness writes locally via FUSE (bypassing URL encoding), but reads remotely via CLI — this asymmetry is exactly what the test is designed to catch. Just confirming the test intent is correct and this isn't a harness bug.
There was a problem hiding this comment.
Confirmed. This asymmetry is intentional: the case writes through FUSE and reads through the CLI specifically to catch remote path encoding/round-trip issues.
| } | ||
| args = append(args, ":"+remoteRoot, mountpoint) | ||
| cmd := exec.CommandContext(ctx, drive9Bin, args...) | ||
| cmd.Env = withEnv(os.Environ(), env) |
There was a problem hiding this comment.
Mount log file leaked on process exit — The goroutine:
go func() {
_ = cmd.Wait()
_ = logFile.Close()
}()If StartMount returns successfully but the caller never waits for the mount to exit (e.g., it calls KillMount which sends SIGTERM), the goroutine is orphaned until the process actually exits. This is fine in practice since kill will cause Wait to return, but if kill fails silently, the file descriptor leaks until process exit.
There was a problem hiding this comment.
Agree the risk is bounded by the mount process lifetime. The normal Stop/Kill paths should cause cmd.Wait() to return and close the log file; if kill itself fails, the mount process is still alive and the descriptor remains tied to that process lifecycle. No code change in this batch.
| if err != nil { | ||
| return err | ||
| } | ||
| if err := waitLocalContent(ctx, c, local, f.Content); err != nil { |
There was a problem hiding this comment.
provision() uses http.DefaultClient without timeout — http.DefaultClient has no timeout, so a hanging server will block forever. Set a client-level timeout or use the context deadline:
client := &http.Client{Timeout: 30 * time.Second}There was a problem hiding this comment.
Addressed in e6bb4d0. The runner now uses a dedicated http.Client with a 30s timeout for provision/status HTTP calls while preserving request contexts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@e2e/agent-harness/schemas/manifest.schema.json`:
- Around line 64-69: The manifest schema currently requires the "status"
property on each case which is too strict; update the JSON schema for the
"cases" items to remove "status" from the "required" array so that
"cases[].status" becomes optional (adjust the same change in the other identical
block around the 90-98 section), ensuring the schema still declares "status" in
"properties" but does not list it under "required".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e0b3999-2d73-4ce0-8cab-ae2ebb20032f
📒 Files selected for processing (12)
docs/design/drive9-agent-harness-v2-plan.mde2e/agent-harness/internal/mountproc/mountproc.goe2e/agent-harness/internal/mountproc/mountproc_test.goe2e/agent-harness/internal/report/report.goe2e/agent-harness/internal/report/report_test.goe2e/agent-harness/internal/runner/runner.goe2e/agent-harness/internal/runner/runner_test.goe2e/agent-harness/internal/safety/safety.goe2e/agent-harness/schemas/case.schema.jsone2e/agent-harness/schemas/gating.schema.jsone2e/agent-harness/schemas/manifest.schema.jsone2e/agent-harness/schemas/summary.schema.json
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/agent-harness/schemas/gating.schema.json
- e2e/agent-harness/schemas/case.schema.json
- e2e/agent-harness/internal/mountproc/mountproc.go
- e2e/agent-harness/internal/runner/runner_test.go
- e2e/agent-harness/internal/safety/safety.go
- e2e/agent-harness/internal/report/report.go
- e2e/agent-harness/internal/runner/runner.go
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
PR #427 Round 2 Review
Good progress on the fixes. Several issues properly addressed, but 4 blockers remain unresolved.
Fixed ✅
- Race in
runKillDuringWrite— Properly fixed withwriterFinished/killedflags and correct channel drain logic. provision()HTTP timeout — Fixed withharnessHTTPClient = &http.Client{Timeout: 30 * time.Second}.Generate()regeneration duration — Fixed by readingrun_endevent fromevents.jsonl. Good test added.- Git workflow context — Now consistently uses
gitCtxinstead of mixing parent/child contexts. - Git mutation write errors — Now properly checked instead of silently discarded.
- GC/safety error handling —
IsMountederrors properly surfaced; sentinel errors added. - JSON schemas — Significantly expanded with
additionalProperties: falseand full property definitions. - Good new tests — 5 new test functions covering the fixes.
Still open ❌
B1: --sync-mode deprecated (CRITICAL) — mountproc.go:StartMount line 2005-2007 still passes --sync-mode. PR #431 merged --durability to main. The harness is broken on current main.
B2: appendJSONL not thread-safe — No mutex added to Recorder. runParallelWrites goroutines can still call rec.Failure() concurrently → interleaved JSONL lines.
B3: runID() second-granularity collision — Still time.Now().UTC().Format("20060102T150405Z"). Two runs in same second → same artifact dir, same remote root, silent corruption.
B4: ValidateRelativeSlashPath over-restrictive for path_matrix — Still uses path.Clean(p) != p check that rejects paths with consecutive spaces, trailing dots, etc. — exactly the edge cases path_matrix exists to test. The regression test path "space name unicode-测试 punct_!@%.txt" happens to survive path.Clean, but a path like "double space.txt" would be rejected.
Please fix B1-B4 and this is ready to approve.
| return nil, err | ||
| } | ||
| logFile, err := os.OpenFile(logPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o644) | ||
| if err != nil { |
There was a problem hiding this comment.
Still uses --sync-mode (deprecated by PR #431) — Line 72 in the new file:
args = append(args, "--sync-mode", syncMode)PR #431 replaced --sync-mode + --write-policy with a single --durability flag. This will break on any build that includes #431. Need to either:
- Map case
sync_mode→--durabilityprofiles, or - Keep
--sync-modeif the CLI still accepts it as a hidden/deprecated alias
Verify against current main CLI.
There was a problem hiding this comment.
Already addressed in 53692d5 and still present after fd39e97. StartMount no longer appends --sync-mode directly; it calls mountSyncArgs, detects whether drive9 mount --help exposes --durability or legacy --sync-mode, and maps case sync_mode values for current main (strict -> fsync, interactive -> interactive, auto -> auto).
| resp, err := harnessHTTPClient.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
runID() still second-granularity — Two harness invocations within the same second produce identical run IDs → same drive9-agent-test-* directory, same remote root base, silent data corruption. Add a random suffix, e.g.:
func runID() string {
b := make([]byte, 4)
_, _ = rand.Read(b)
return time.Now().UTC().Format("20060102T150405Z") + "-" + hex.EncodeToString(b)
}There was a problem hiding this comment.
Addressed in fd39e97. runID() now appends a random 4-byte hex suffix to the compact UTC timestamp, so same-second harness invocations do not collide on run directory or remote root names. Added focused coverage in TestRunIDIncludesRandomSuffix.
| KnownBugFixedCandidate int `json:"known_bug_fixed_candidate"` | ||
| NonGating int `json:"non_gating"` | ||
| BlockingFailures []Failure `json:"blocking_failures"` | ||
| } |
There was a problem hiding this comment.
appendJSONL still not safe for concurrent use — Recorder has no mutex. runParallelWrites spawns goroutines that can call recordOracleFailure → rec.Failure() → appendJSONL concurrently. While O_APPEND provides some atomicity on POSIX, the JSON marshal + f.Write(append(b, '\n')) sequence is not atomic — two goroutines can interleave mid-line.
Add a sync.Mutex to Recorder and lock around appendJSONL calls, or serialize writes through a channel.
There was a problem hiding this comment.
Addressed in fd39e97. Recorder now has a sync.Mutex and serializes manifest, event, failure, and metric artifact writes. Added TestRecorderConcurrentWrites to exercise concurrent Event, Failure, and Metric writes and verify the JSONL artifacts remain parseable.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
PR #427 — Round 2 Review: REVISE
Head: e6bb4d0 (was dc0f1ac). 29 files, +5398/-1.
What's fixed
- ✅ dat9-dev1:
http.DefaultClienttimeout —harnessHTTPClientwith 30s timeout replaceshttp.DefaultClient. Good. - ✅ dat9-dev1:
Generate()duration on regeneration —readRunEndreads lastrun_endevent fromevents.jsonl. Correct. - ✅ dat9-dev1:
runKillDuringWriterace — improved withkilled/writerFinishedtracking. Thedefaultbranch after kill is acceptable for fault-suite semantics (inconclusive classification).
Withdrawn (my error in Round 1)
B3 (— Withdrawn.shouldRemoveMountpointbroken fordoctor_fuse)caseHadFailures(rec, c.ID)readsfailures.jsonlas a backup, so even withretErr == nil, recorded failures prevent mountpoint removal underretain_on_failure. Logic is correct.B6 (— Withdrawn. Go struct value copy preserves all fields includingGenerate()zeroes Suite field)Suite. The failure iteration only overwritesID,ExpectedOutcome,Status—Suiteis preserved from the originalmanifest.Casesentry.
Still open — blockers
B1. ValidateRoot semantic mismatch — NOT FIXED
Still a single function for both local filesystem roots and remote-path roots with filesystem-centric error messages. Low severity but worth splitting to avoid future misuse.
B2. runID() collision — NOT FIXED
Still time.Now().UTC().Format("20060102T150405Z") — second granularity. Two runs within the same second get the same artifact directory. Add random suffix.
B4. ValidateRelativeSlashPath over-restrictive — NOT FIXED
path.Clean(p) != p still rejects paths with consecutive spaces, trailing dots, etc. — exactly the edge cases path_matrix exists to test.
B5. provision() nil body — NOT FIXED
Still http.NewRequestWithContext(ctx, POST, ..., nil). No Content-Type, no body.
B7. --sync-mode deprecated — NOT FIXED (dat9-dev1 catch)
StartMount() at mountproc.go still passes --sync-mode. PR #431 merged --durability. The harness is broken on current main.
B8. appendJSONL not thread-safe — NOT FIXED (dat9-dev1 catch)
Recorder has no mutex. runParallelWrites goroutines can call recordOracleFailure → rec.Failure() → appendJSONL concurrently. The local sync.Mutex in runParallelWrites only protects the local errs slice, not the Recorder's file writes.
B9. Manifest written only after all cases — NOT FIXED (dat9-dev1 catch)
WriteManifest at line 2867 runs only after the case loop. Crash mid-run loses all metadata including generated mountpoints and remote roots, so gc can't clean up.
Verdict: REVISE
5 blockers remain from Round 1 (B1, B2, B4, B5, B7) and 2 from dat9-dev1 (B8, B9). B2 and B7 are the most critical — B2 is a silent data corruption, B7 means the harness is already broken on main.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@e2e/agent-harness/internal/runner/runner.go`:
- Around line 197-199: When WriteManifest fails you currently return an empty
string which loses the runDir needed for debugging; change the error return in
the rec.WriteManifest(manifest) failure path to return the existing runDir and
the error instead of "", err. Locate the rec.WriteManifest(manifest) call in
runner.go (the surrounding function that returns (string, error)) and replace
the return so the function returns runDir, err.
- Line 824: The write to the held-open file currently discards the error from
f.WriteString("held open by harness\n"); capture the returned error (e.g., _,
err := f.WriteString(...)) and handle it instead of throwing it away — at
minimum log it or return it up the call path so I/O failures are visible (use
the existing test/harness logger or propagate the error from the surrounding
function). Also follow the explicit-ignore guideline elsewhere by using `_ =
s.Close()` when intentionally ignoring Close errors. Locate the f.WriteString
call and modify its error handling accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5e0ab77-ac94-4f70-b7d5-43d0f95ecf2b
📒 Files selected for processing (8)
docs/design/drive9-agent-harness-v2-plan.mde2e/agent-harness/internal/mountproc/mountproc.goe2e/agent-harness/internal/mountproc/mountproc_test.goe2e/agent-harness/internal/report/report.goe2e/agent-harness/internal/report/report_test.goe2e/agent-harness/internal/runner/runner.goe2e/agent-harness/internal/runner/runner_test.goe2e/agent-harness/schemas/manifest.schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/agent-harness/internal/mountproc/mountproc.go
- e2e/agent-harness/internal/report/report.go
|
@qiffang follow-up on #427 (review):
B1 ( |
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Blocking issue on the current PR merge ref:
pkg/fuse/dat9fs.go:1118does not compile. The merge result callsfs.perf.readRetrySuccess.add(1), butatomic.Uint64exposesAdd, notadd:
pkg/fuse/dat9fs.go:1118:29: fs.perf.readRetrySuccess.add undefined (type "sync/atomic".Uint64 has no field or method add, but does have method Add)
This is not just a local nit: GitHub ci is failing at the same compile/lint gate, and this is the merge ref GitHub will test before merge. The branch needs to be rebased/merged with current main and the conflict result fixed to Add(1).
- Required checks are still red.
drive9-server-local e2e smokealso failed (CLI smoke did not succeed,FUSE release gate did not succeed). This may be downstream of the compile failure, but it still needs to be green before merging.
Local validation:
PASS: /usr/local/go/bin/go test ./e2e/agent-harness/... -count=1
FAIL: /usr/local/go/bin/go test ./pkg/fuse -run TestNonExistent -count=1
I did not find a harness-specific correctness blocker in the files I reviewed, but the PR cannot be approved while the merge ref does not compile and required checks are red.
aedfd74 to
07f3069
Compare
1c882b7 to
b2c9e70
Compare
Introduce the reusable e2e agent harness with smoke, regression, stress, and fault suites; structured artifacts and schemas; report/gating generation; GC and server-evidence command surfaces; and the design proposal under docs/design.
b2c9e70 to
733274a
Compare
2 participants
Summary
e2e/agent-harnesswithpreflight,run,report,gc, andcollect-server-evidencecommands.docs/design/drive9-agent-harness-v2-plan.md.Validation
go test ./e2e/agent-harness/...go build -o /tmp/drive9-agent-harness-check ./e2e/agent-harness/cmd/drive9-agent-harnessSummary by CodeRabbit
New Features
Documentation
Tests