Conversation
…axonomy (#380) Slice S1 of the deterministic real-process TUI harness: closed contract types, a bounded strict JSON reader (duplicate-key rejection, decimal integers, inclusive depth/member/array/string/input limits), scalar validators (RelPath, EnvName, capture Id, strict base64, secrets), workspace-prefix-only interpolation grammar, cross-field semantic rules, and the HAR-E001..E007 taxonomy with the 0/2/4/124 exit mapping. Schema-1 is the only accepted input by explicit decision: missing or wrong schema is HAR-E001. No legacy adapter or compatibility shim; the shipped pre-schema scenarios migrate forward in CW-00b (#397).
Slice S2: mode-0700 unique workspace with no-follow ancestor identity verification (HAR-E004 on symlink swap or replaced-directory races, no check-then-follow path), fixture materialization with declared modes, the empty-base deterministic environment rooted in the workspace, and the longest-first secret redactor applied at the serialization boundary.
Slice S3: the jefe-capture-shim fixture locates its behavior file beside its own executable (no env, no PATH lookup, no shell), claims a start ordinal atomically via create_new lock files (bounding processes per capture at 32), records raw argv, sorted env pairs, cwd, bounded stdin, configured streams and exit, and supports hang/spawn_child_hang for timeout and reaping tests. Registration materializes the shim into the workspace through the contained no-follow path; assert-capture compares every recorded field and raises HAR-E006 on mismatch.
Slice S4: PtySession owns the portable-pty launch (setsid gives the app its own process group), input delivery, the merged output stream, an alacritty_terminal grid for exact-size frames, resize, and escalating TERM/KILL/verify teardown bounded at 2s per phase via fixed-path /bin/kill (no shell, no PATH lookup, no unsafe). The runner executes the validated plan sequentially: bounded literal waits, exact-dimension resize acknowledgement, restart preserving durable files while replacing/reaping the process group, capture assertion, and failure-stop with workspace retention. A closed key-encoding table and the jefe-harness-probe fixture app complete the slice; e2e tests cover CW00-03/05/06/08/09 against real processes.
Slice S5: the tmux_scenario binary reads one schema-1 scenario, runs it, prints exactly one deterministic redacted report, and returns 0/2/4/124. Schema-1 is the only accepted input; pre-schema documents fail with HAR-E001 (no legacy adapter — migration is issue #397). The eight CW00 ledger fixtures live under dev-docs/tmux-scenarios/v1/ and execute through tests/harness_v1_fixtures.rs against the real runner. Capture expectations normalize workspace-rooted recorded values to the ${workspace} token so fixtures can state exact bytes despite unique workspace paths, and listed env pairs match by name while deterministic base variables remain permitted. The probe sequences RUN output so consecutive identical commands stay awaitable, and the workspace root is canonicalized so recorded process fields share its prefix. Docs: the schema-1 grammar, ownership map, containment algorithm, exits, limits, and the bounded-literal-wait rule in tmux-harness.md plus the standards index.
|
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:
📝 WalkthroughWalkthroughThe PR adds a schema-1 deterministic real-process TUI harness with strict parsing, contained workspaces, hermetic environments, capture shims, PTY execution, redacted reports, CLI binaries, scenario fixtures, and unit/integration coverage. ChangesSchema-1 deterministic harness
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
OpenCodeReview — PR #402
OCR stderr excerptOCR preview stderr excerpt |
acoliver
added
the
review-ready
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/harness/v1/workspace_tests.rs (1)
120-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a symlink-at-target test for
remove(), mirroring thewrite_fileattack test.
symlink_target_write_is_containment_errorproveswrite_filewon't follow a symlink at the target path, butremove()'s equivalent branch (symlink_metadata+remove_filewhen the target itself is a symlink) has no analogous test here. Given the file's stated purpose (CW00-07 containment coverage), a mirrored test would directly proveremove()never follows a symlink at the removal target and leaves anything outside the workspace untouched.Also applies to: 197-213
🤖 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 `@src/harness/v1/workspace_tests.rs` around lines 120 - 139, Add a containment test alongside symlink_target_write_is_containment_error for remove(), creating an outside victim, placing a workspace symlink at the removal target, and calling remove() on it. Assert the operation returns the expected containment error and verify the outside victim remains unchanged, covering the symlink_metadata/remove_file branch.
🤖 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 `@src/harness/v1/parse_step.rs`:
- Around line 129-169: Validate the parsed Step::Key in parse_key or
semantic::validate by calling keys::encode for the key and modifiers before
execution begins. Propagate encoding errors as HarnessError syntax/validation
errors, while preserving the existing duplicate and unknown-modifier checks, so
unsupported keys and modifier combinations fail before runner.rs performs side
effects.
- Around line 315-331: Update parse_byte_pairs to use the shared parse_env_list
validator for CaptureExpectation.env instead of manually parsing entries. Remove
the custom name/value loop and preserve the existing Result<Vec<EnvVar>,
HarnessError> behavior so MAX_ENV and Env validation are consistently enforced.
In `@src/harness/v1/semantic.rs`:
- Around line 25-36: Extend semantic validation around the path collection in
the workspace validation flow to reject any configured path nested beneath a
previously configured file path, including file/descendant and file/directory
collisions. Preserve exact-duplicate detection and file-size checks, and keep
validation deterministic without moving materialization side effects into
src/**/*.rs.
In `@src/harness/v1/workspace.rs`:
- Around line 292-328: Update open_dir_nofollow to classify only ELOOP failures
as HarnessError::containment and route all other I/O errors through the existing
process-error classification used by classify_open_error. Preserve the no-follow
directory-open behavior and the existing verify_ancestor callers.
---
Nitpick comments:
In `@src/harness/v1/workspace_tests.rs`:
- Around line 120-139: Add a containment test alongside
symlink_target_write_is_containment_error for remove(), creating an outside
victim, placing a workspace symlink at the removal target, and calling remove()
on it. Assert the operation returns the expected containment error and verify
the outside victim remains unchanged, covering the symlink_metadata/remove_file
branch.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6668b34-6b85-4577-ab5a-fb379673869d
📒 Files selected for processing (43)
Cargo.tomldev-docs/standards/testing-and-quality.mddev-docs/testing/tmux-harness.mddev-docs/tmux-scenarios/v1/harness-capture.jsondev-docs/tmux-scenarios/v1/harness-containment.jsondev-docs/tmux-scenarios/v1/harness-interpolation.jsondev-docs/tmux-scenarios/v1/harness-limits.jsondev-docs/tmux-scenarios/v1/harness-redaction.jsondev-docs/tmux-scenarios/v1/harness-resize-restart.jsondev-docs/tmux-scenarios/v1/harness-schema-all-ops.jsondev-docs/tmux-scenarios/v1/harness-timeout.jsonproject-plans/issue380-plan.mdsrc/bin/jefe-capture-shim.rssrc/bin/jefe-harness-probe.rssrc/bin/tmux_scenario.rssrc/harness/mod.rssrc/harness/v1/capture.rssrc/harness/v1/capture_tests.rssrc/harness/v1/contract.rssrc/harness/v1/env.rssrc/harness/v1/error.rssrc/harness/v1/fields.rssrc/harness/v1/interp.rssrc/harness/v1/interp_tests.rssrc/harness/v1/json.rssrc/harness/v1/json_tests.rssrc/harness/v1/keys.rssrc/harness/v1/limits.rssrc/harness/v1/mod.rssrc/harness/v1/parse.rssrc/harness/v1/parse_step.rssrc/harness/v1/parse_tests.rssrc/harness/v1/pty.rssrc/harness/v1/redact.rssrc/harness/v1/report.rssrc/harness/v1/runner.rssrc/harness/v1/semantic.rssrc/harness/v1/validate.rssrc/harness/v1/validate_tests.rssrc/harness/v1/workspace.rssrc/harness/v1/workspace_tests.rstests/harness_v1.rstests/harness_v1_fixtures.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: OpenCodeReview
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Apply Jefe's Rust standards. Prefer explicit domain types; use Option for
absence and Result with typed errors for fallible operations. Require no
unwrap or expect in production paths and no unsafe code. Check Unix,
macOS, and native Windows behavior where process, path, terminal, or
runtime boundaries are changed.
Files:
src/harness/mod.rssrc/harness/v1/interp_tests.rssrc/harness/v1/keys.rssrc/harness/v1/interp.rssrc/harness/v1/redact.rssrc/harness/v1/env.rssrc/harness/v1/mod.rssrc/harness/v1/capture_tests.rssrc/harness/v1/error.rssrc/harness/v1/json_tests.rssrc/harness/v1/workspace_tests.rssrc/harness/v1/fields.rssrc/bin/tmux_scenario.rssrc/harness/v1/validate_tests.rssrc/bin/jefe-capture-shim.rssrc/harness/v1/json.rssrc/harness/v1/semantic.rssrc/harness/v1/report.rssrc/harness/v1/contract.rstests/harness_v1.rstests/harness_v1_fixtures.rssrc/harness/v1/parse_step.rssrc/harness/v1/parse_tests.rssrc/harness/v1/validate.rssrc/harness/v1/capture.rssrc/bin/jefe-harness-probe.rssrc/harness/v1/limits.rssrc/harness/v1/pty.rssrc/harness/v1/runner.rssrc/harness/v1/parse.rssrc/harness/v1/workspace.rs
src/**/*.rs
⚙️ CodeRabbit configuration file
src/**/*.rs: Preserve the documented ownership boundaries and unidirectional data
flow. Keep state transitions deterministic and side effects at runtime,
persistence, GitHub, or other boundary modules. Check cancellation,
stale-result, cleanup, and durability behavior for external work.
Files:
src/harness/mod.rssrc/harness/v1/interp_tests.rssrc/harness/v1/keys.rssrc/harness/v1/interp.rssrc/harness/v1/redact.rssrc/harness/v1/env.rssrc/harness/v1/mod.rssrc/harness/v1/capture_tests.rssrc/harness/v1/error.rssrc/harness/v1/json_tests.rssrc/harness/v1/workspace_tests.rssrc/harness/v1/fields.rssrc/bin/tmux_scenario.rssrc/harness/v1/validate_tests.rssrc/bin/jefe-capture-shim.rssrc/harness/v1/json.rssrc/harness/v1/semantic.rssrc/harness/v1/report.rssrc/harness/v1/contract.rssrc/harness/v1/parse_step.rssrc/harness/v1/parse_tests.rssrc/harness/v1/validate.rssrc/harness/v1/capture.rssrc/bin/jefe-harness-probe.rssrc/harness/v1/limits.rssrc/harness/v1/pty.rssrc/harness/v1/runner.rssrc/harness/v1/parse.rssrc/harness/v1/workspace.rs
tests/**/*.rs
⚙️ CodeRabbit configuration file
tests/**/*.rs: Review changed tests and behavioral coverage as first-class code. Tests
should prove observable success and failure behavior without relying on
arbitrary sleeps, mock call counts, host load, or panic-driven setup.
Files:
tests/harness_v1.rstests/harness_v1_fixtures.rs
🧠 Learnings (2)
📚 Learning: 2026-04-04T21:05:37.792Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 36
File: src/github/mod.rs:537-542
Timestamp: 2026-04-04T21:05:37.792Z
Learning: In Rust, it’s acceptable for function parameters to be intentionally unused when their names start with an underscore (e.g., `_cursor: Option<&str>`). If the parameter name is underscore-prefixed, treat it as a deliberate forward-compatibility/API-consistency choice and do not raise an “unused parameter”/dead code warning during code review.
Applied to files:
src/harness/mod.rssrc/harness/v1/interp_tests.rssrc/harness/v1/keys.rssrc/harness/v1/interp.rssrc/harness/v1/redact.rssrc/harness/v1/env.rssrc/harness/v1/mod.rssrc/harness/v1/capture_tests.rssrc/harness/v1/error.rssrc/harness/v1/json_tests.rssrc/harness/v1/workspace_tests.rssrc/harness/v1/fields.rssrc/bin/tmux_scenario.rssrc/harness/v1/validate_tests.rssrc/bin/jefe-capture-shim.rssrc/harness/v1/json.rssrc/harness/v1/semantic.rssrc/harness/v1/report.rssrc/harness/v1/contract.rstests/harness_v1.rstests/harness_v1_fixtures.rssrc/harness/v1/parse_step.rssrc/harness/v1/parse_tests.rssrc/harness/v1/validate.rssrc/harness/v1/capture.rssrc/bin/jefe-harness-probe.rssrc/harness/v1/limits.rssrc/harness/v1/pty.rssrc/harness/v1/runner.rssrc/harness/v1/parse.rssrc/harness/v1/workspace.rs
📚 Learning: 2026-06-22T22:25:52.693Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 81
File: src/app_init.rs:11-13
Timestamp: 2026-06-22T22:25:52.693Z
Learning: In Rust, when you call a method that is defined on a trait (e.g., `spawn_session` from `jefe::runtime::RuntimeManager`) on a concrete type (e.g., `TmuxRuntimeManager`), ensure the defining trait is imported into scope (`use ...::RuntimeManager`) if it is not already in scope. Otherwise the call may fail with a “method not found” error because Rust’s method-call resolution requires the trait to be in scope.
Applied to files:
src/harness/mod.rssrc/harness/v1/interp_tests.rssrc/harness/v1/keys.rssrc/harness/v1/interp.rssrc/harness/v1/redact.rssrc/harness/v1/env.rssrc/harness/v1/mod.rssrc/harness/v1/capture_tests.rssrc/harness/v1/error.rssrc/harness/v1/json_tests.rssrc/harness/v1/workspace_tests.rssrc/harness/v1/fields.rssrc/bin/tmux_scenario.rssrc/harness/v1/validate_tests.rssrc/bin/jefe-capture-shim.rssrc/harness/v1/json.rssrc/harness/v1/semantic.rssrc/harness/v1/report.rssrc/harness/v1/contract.rstests/harness_v1.rstests/harness_v1_fixtures.rssrc/harness/v1/parse_step.rssrc/harness/v1/parse_tests.rssrc/harness/v1/validate.rssrc/harness/v1/capture.rssrc/bin/jefe-harness-probe.rssrc/harness/v1/limits.rssrc/harness/v1/pty.rssrc/harness/v1/runner.rssrc/harness/v1/parse.rssrc/harness/v1/workspace.rs
🪛 LanguageTool
project-plans/issue380-plan.md
[style] ~57-~57: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tring splitting, or host PATH lookup. - No network helpers, no async runtime, no n...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o async runtime, no new dependencies. - No changes to .github/workflows, quality...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~58-~58: The official name of this software platform is spelled with a capital “H”.
Context: ...e, no new dependencies. - No changes to .github/workflows, quality gates, or threshold...
(GITHUB)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...flows, quality gates, or thresholds. - No edits to shipped dev-docs/tmux-scenari...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (45)
src/harness/v1/capture.rs (2)
144-153: 🩺 Stability & AvailabilityRecords loading stops at the first ordinal gap.
load_recordsbreaks as soon as neither{n}.jsonnor{n}.start.jsonexists. This is correct only under the invariant that every claimed ordinal writes a start record before the shim can hang or be reaped. If a shim is killed in the window between claiming its.lockand writing.start.json, that slot leaves a gap and all higher-ordinal records are silently dropped from the report. Worth confirming the claim→start write is guaranteed to be non-interruptible by the harness escalation path.
181-182: 🎯 Functional Correctness
invocationis already constrained to1+before reaching this code.
src/harness/v1/parse_step.rsparsesCaptureExpectation.invocationwith(1, u64::MAX), soexpectation.invocation - 1cannot underflow on the normal path.> Likely an incorrect or invalid review comment.src/bin/jefe-capture-shim.rs (1)
35-67: LGTM!src/harness/v1/capture_tests.rs (1)
52-147: LGTM!src/harness/v1/runner.rs (1)
49-90: LGTM!src/bin/jefe-harness-probe.rs (1)
20-136: LGTM!tests/harness_v1.rs (1)
206-249: LGTM!src/bin/tmux_scenario.rs (1)
37-167: LGTM!dev-docs/tmux-scenarios/v1/harness-timeout.json (1)
1-39: LGTM!tests/harness_v1_fixtures.rs (1)
119-160: LGTM!src/harness/v1/pty.rs (2)
416-442: 🩺 Stability & Availability
kill -- -<pgid>is valid on macOS, so this portability concern doesn’t hold.> Likely an incorrect or invalid review comment.
18-23: 🩺 Stability & AvailabilityNo change needed here.
src/harness/v1/redact.rs (1)
1-121: LGTM!src/harness/v1/report.rs (1)
1-199: LGTM!src/harness/mod.rs (1)
32-32: LGTM!dev-docs/tmux-scenarios/v1/harness-capture.json (1)
1-63: LGTM!dev-docs/tmux-scenarios/v1/harness-containment.json (1)
1-33: LGTM!dev-docs/tmux-scenarios/v1/harness-interpolation.json (1)
1-43: LGTM!dev-docs/tmux-scenarios/v1/harness-limits.json (1)
1-9: LGTM!dev-docs/tmux-scenarios/v1/harness-redaction.json (1)
1-25: LGTM!dev-docs/tmux-scenarios/v1/harness-resize-restart.json (1)
1-40: LGTM!dev-docs/tmux-scenarios/v1/harness-schema-all-ops.json (1)
1-84: LGTM!Cargo.toml (1)
76-87: LGTM!project-plans/issue380-plan.md (1)
1-135: LGTM!src/harness/v1/contract.rs (2)
1-95: LGTM!Also applies to: 122-249
113-121: 🎯 Functional Correctness
RelPath::derivedis fine under the current gatingRelPath::derivedis only called from#[cfg(unix)]code paths insrc/harness/v1, so thecfg(unix)constructor does not create a non-Unix build break.> Likely an incorrect or invalid review comment.src/harness/v1/error.rs (1)
1-135: LGTM!src/harness/v1/fields.rs (1)
1-158: LGTM!src/harness/v1/json.rs (1)
1-349: LGTM!src/harness/v1/json_tests.rs (1)
1-137: LGTM!src/harness/v1/parse.rs (1)
1-268: LGTM!src/harness/v1/parse_tests.rs (1)
1-308: LGTM!src/harness/v1/validate.rs (1)
1-182: LGTM!src/harness/v1/validate_tests.rs (1)
1-136: LGTM!dev-docs/testing/tmux-harness.md (1)
343-484: LGTM!src/harness/v1/keys.rs (1)
1-157: LGTM!src/harness/v1/limits.rs (1)
1-46: LGTM!src/harness/v1/interp.rs (1)
1-78: LGTM!src/harness/v1/interp_tests.rs (1)
1-77: LGTM!src/harness/v1/parse_step.rs (1)
1-128: LGTM!Also applies to: 171-314, 333-345
dev-docs/standards/testing-and-quality.md (1)
41-50: LGTM!src/harness/v1/env.rs (1)
1-116: LGTM!src/harness/v1/workspace.rs (1)
1-361: LGTM! Rest of the containment/materialization logic (resolve/mkdir/write_file/remove/read_file/exists/identity tracking) is correctly implemented and matches the documented invariants.src/harness/v1/workspace_tests.rs (1)
1-236: LGTM!src/harness/v1/mod.rs (1)
10-30: 🎯 Functional Correctness
capturedoes not need a module-level#[cfg(unix)]
registerandensure_records_dirsare already Unix-gated; the rest ofcapture.rsis cross-platform.> Likely an incorrect or invalid review comment.
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 `@src/harness/v1/capture.rs`:
- Around line 109-152: In record_terminated_signals, re-check whether the
ordinal’s done record exists immediately before write_record_atomic updates the
start record. Skip the write when done has appeared, preserving the existing
signal and process checks while preventing stale start records from being marked
after normal completion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e91d09c0-2231-48d8-bc48-41d87a69abdc
📒 Files selected for processing (12)
src/bin/jefe-capture-shim.rssrc/bin/jefe-harness-probe.rssrc/bin/tmux_scenario.rssrc/harness/v1/capture.rssrc/harness/v1/capture_tests.rssrc/harness/v1/parse_tests.rssrc/harness/v1/pty.rssrc/harness/v1/redact.rssrc/harness/v1/runner.rssrc/harness/v1/semantic.rssrc/harness/v1/workspace.rstests/harness_v1.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/bin/jefe-capture-shim.rs
- src/harness/v1/redact.rs
- src/harness/v1/capture_tests.rs
- tests/harness_v1.rs
- src/bin/tmux_scenario.rs
- src/harness/v1/workspace.rs
- src/harness/v1/pty.rs
- src/harness/v1/parse_tests.rs
- src/harness/v1/semantic.rs
- src/harness/v1/runner.rs
- src/bin/jefe-harness-probe.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Native Windows (MSVC + psmux)
- GitHub Check: Coverage gate
- GitHub Check: OpenCodeReview
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Apply Jefe's Rust standards. Prefer explicit domain types; use Option for
absence and Result with typed errors for fallible operations. Require no
unwrap or expect in production paths and no unsafe code. Check Unix,
macOS, and native Windows behavior where process, path, terminal, or
runtime boundaries are changed.
Files:
src/harness/v1/capture.rs
src/**/*.rs
⚙️ CodeRabbit configuration file
src/**/*.rs: Preserve the documented ownership boundaries and unidirectional data
flow. Keep state transitions deterministic and side effects at runtime,
persistence, GitHub, or other boundary modules. Check cancellation,
stale-result, cleanup, and durability behavior for external work.
Files:
src/harness/v1/capture.rs
🧠 Learnings (2)
📚 Learning: 2026-04-04T21:05:37.792Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 36
File: src/github/mod.rs:537-542
Timestamp: 2026-04-04T21:05:37.792Z
Learning: In Rust, it’s acceptable for function parameters to be intentionally unused when their names start with an underscore (e.g., `_cursor: Option<&str>`). If the parameter name is underscore-prefixed, treat it as a deliberate forward-compatibility/API-consistency choice and do not raise an “unused parameter”/dead code warning during code review.
Applied to files:
src/harness/v1/capture.rs
📚 Learning: 2026-06-22T22:25:52.693Z
Learnt from: acoliver
Repo: vybestack/llxprt-jefe PR: 81
File: src/app_init.rs:11-13
Timestamp: 2026-06-22T22:25:52.693Z
Learning: In Rust, when you call a method that is defined on a trait (e.g., `spawn_session` from `jefe::runtime::RuntimeManager`) on a concrete type (e.g., `TmuxRuntimeManager`), ensure the defining trait is imported into scope (`use ...::RuntimeManager`) if it is not already in scope. Otherwise the call may fail with a “method not found” error because Rust’s method-call resolution requires the trait to be in scope.
Applied to files:
src/harness/v1/capture.rs
🔇 Additional comments (3)
src/harness/v1/capture.rs (3)
11-13: LGTM!Also applies to: 51-66, 68-107
413-441: 🎯 Functional CorrectnessNo change needed for the let-chain syntax.
Cargo.tomlalready uses edition 2024, and CI runs on the stable Rust toolchain, soif let ... && ...is valid here.> Likely an incorrect or invalid review comment.
154-175: 🩺 Stability & AvailabilityUnix-only capture path is intentional —
src/harness/v1/contract.rsonly modelsPlatform::MacosandPlatform::Linux, so this helper does not need a Windows fallback here.> Likely an incorrect or invalid review comment.
OCR finding dispositionReviewed all 45 OCR findings against the accepted schema-1 contract and source. Fixed
Rejected as incorrect, contradicted by contract, or already covered
CodeRabbit follow-upCodeRabbit independently found four valid parser/containment issues. They were fixed in 5861f8f, each thread received a source-grounded response, and all four threads were resolved. Its symlink-remove test suggestion was adapted to the actual safety contract: remove unlinks the workspace symlink and proves the outside victim is untouched. |
|
Temporarily closing to refresh stale GitHub head metadata; branch and commits are unchanged. |
|
Refreshing stale GitHub head metadata after the final reviewed commit; branch history is unchanged. |
|
Final OCR cycle disposition (reviewed against the closed issue contract and exact source): Fixed in 96e646d:
Rejected as contradicted by the contract/source:
Rejected/deferred as bounded optional work rather than correctness defects:
No further OCR review/remediation cycle will be started. Remaining suggestions are explicitly rejected or deferred under the bounded issue workflow. |
1 participant
Summary
Scope boundary
Schema 1 is the only input accepted by the new runner. This PR adds no legacy adapter, lowering pass, compatibility shim, dual parser, or format-detection path.
Existing pre-schema scenarios and their harness remain temporarily unchanged so this foundational PR has one executable boundary. Immediate follow-up #397 migrates every shipped scenario forward to schema 1, updates all callers, proves equivalence, and deletes the superseded harness/parser. No runtime compatibility path is permitted there either.
The issue delivery exceeds the normal size guardrail (43 issue files and about 6.9k added lines before mainline integration). The user explicitly approved one cohesive PR and declined stacked PRs. The implementation is separated into focused modules and green commits.
Acceptance evidence
Verification
Fixes #380
Summary by CodeRabbit