fix: repair OpenTUI CLI contract, sync-branch integrity, and output/skill-source hardening#2
Merged
Merged
Conversation
Summary by CodeRabbit
WalkthroughAdds push-mode control for sync migration, sanitizes CLI/TUI terminal output, removes a current-directory skills source fallback, and refactors OpenTUI to fetch data through the ChangesSync Branch Migration
Terminal Output Sanitization
Skills CWD Source Fix
OpenTUI Async Refactor
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@plans/001-event-log-append-fast-path.md`:
- Around line 173-184: The new tail-based logic in prepare_event_file_for_append
must preserve CRLF handling by normalizing a trailing carriage return before
attempting JSON parsing of the final non-empty line. Update the parser in
src/store/events.rs so it strips a terminal \r from the tail line before
validating/truncating, and add a regression test covering a CRLF-terminated
event log to ensure append still succeeds instead of treating it as corrupt.
In `@plans/003-index-duplicate-gate.md`:
- Around line 20-22: The plan currently depends on
plans/002-query-without-eager-clones.md even though this request-scoped index
built from borrowed Task references appears independent. Update the dependency
entry in the duplicate-gate plan to either remove it or narrow it to only the
specific prerequisite actually needed, so the perf work can proceed without
waiting on the broader clone cleanup.
In `@plans/README.md`:
- Around line 24-34: The consolidated verification list in the README is missing
the required cargo check gate. Update the verification gates near the existing
cargo fmt/clippy/test entries to include cargo check --all-targets, or
explicitly note that this command is only enforced per-step so the plan and
index stay aligned.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ac57704-3309-4f89-b98a-0ddc5283c8d5
📒 Files selected for processing (5)
plans/001-event-log-append-fast-path.mdplans/002-query-without-eager-clones.mdplans/003-index-duplicate-gate.mdplans/004-split-cli-render-module.mdplans/README.md
tsq skills refresh probed the current working directory's SKILLS/ folder as a source root ahead of the trusted exe-relative and embedded copies. Running the command inside an untrusted repo shipping SKILLS/tasque/SKILL.md let that content overwrite managed skill installs in user-global agent directories (~/.claude/skills, ~/.codex, ~/.copilot, ~/.opencode). Remove the CWD source root; explicit sources (TSQ_SKILLS_DIR, source_root_dir) and trusted exe-relative/embedded copies remain unaffected. Adds a regression test proving a malicious SKILLS/tasque/SKILL.md in CWD never reaches a managed target.
Task titles, descriptions, notes, and spec content are attacker-influenceable
and synced across machines via events.jsonl. Human-readable output printed
them raw, so ANSI/OSC escape sequences (cursor moves, window-title rewrites,
OSC-52 clipboard writes) would execute in the terminal of anyone running
`tsq show`/`tsq find` on a synced repo.
Add sanitize_inline/sanitize_block in render.rs, escaping ESC and all other
C0/C1 control chars plus DEL as \x{:02x} (existing \\ \t \r \n handling
preserved). plain_cell now delegates to sanitize_inline. Apply at every
human-print site for title, description, note text, spec content, and
assignee across render.rs, tui_render.rs, and watch.rs (tree/table/inspector/
board views). --json output is untouched — machine consumers still get the
original bytes.
The tasque-events merge driver definition lives only in local git config, written during initial setup_sync_branch/migration. Git config does not travel with clones, so on a freshly cloned machine the cold ensure_worktree path in resolve_effective_root and the explicit tsq sync path never configured it, letting git fall back to a default textual 3-way merge on events.jsonl and corrupting the reader. Configure the driver after ensure_worktree succeeds on the cold resolve path, and re-ensure it in sync_worktree before commit/push so clones that materialized their worktree on a pre-fix binary self-heal.
…after local commit Implicit sync-branch migration (triggered by any data command in a legacy repo) pushed to origin with hard-fail propagation, so read-only commands like `tsq find open` failed outright when offline or when push was rejected. And clear_repo_events ran only after a successful push, leaving stale main-tree events behind on push failure with no automatic retry. Clear main-tree events right after the local worktree commit (data is durable locally at that point), and add PushMode: the implicit resolve path uses BestEffort (push failure becomes a stderr warning suggesting 'tsq sync'), while explicit `tsq migrate` keeps Required hard-fail semantics. `tsq sync` push behavior is unchanged.
BumpyClock
force-pushed
the
plan/perf-architecture-cleanup
branch
from
July 2, 2026 15:58
f0fefc1 to
2971c11
Compare
BumpyClock
changed the title
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/render.rs (1)
164-188: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSanitize
external_refanddiscovered_frombefore printingsrc/cli/render.rsstill writes these CLI-populated fields raw, so terminal escape sequences can slip through here.🤖 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/cli/render.rs` around lines 164 - 188, The CLI rendering in render_task still prints external_ref and discovered_from without escaping, which can allow terminal escape sequences through. Update the task field output logic in render_task to pass both task.external_ref and task.discovered_from through sanitize_inline before printing, matching the handling already used for assignee and description.
🤖 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 @.github/workflows/ci.yml:
- Around line 45-52: The CI job currently rebuilds the tsq binary from scratch
on every run, so add caching for Cargo artifacts to speed up repeated builds.
Update the workflow around the rust-toolchain and cargo build steps to cache the
Cargo registry/git data and the target directory, using stable keys tied to the
Rust toolchain and Cargo.lock so the cache stays valid. Keep the existing cargo
build and bun test flow, but ensure the cached artifacts are restored before the
build and saved afterward.
- Around line 45-47: The CI job is installing an extra Rust toolchain even
though the runner already has Rust available. Update the workflow by removing
the unnecessary dtolnay/rust-toolchain@stable step unless cargo build in this
job truly depends on a specific toolchain or component, and keep the build
focused around the cargo build step in the workflow job.
- Line 45: The CI workflow is using an unpinned dtolnay/rust-toolchain
reference, which should be locked to a specific commit SHA. Update the workflow
entry for dtolnay/rust-toolchain in the CI configuration to use the reviewed
pinned SHA instead of `@stable` so the action version cannot change unexpectedly.
In `@src/app/sync.rs`:
- Around line 194-198: The best-effort push warning in `sync` writes git-derived
`remote` and `error.message` directly to stderr, bypassing terminal sanitization
and risking control-sequence injection. Update the warning path in the
`git::push_current_set_upstream` error branch to go through the TTY-aware
render/style sanitization layer used for terminal output, or explicitly escape
both interpolated values before calling `eprintln!`. Keep the fix localized to
the `sync` flow so the warning remains safe while preserving the existing
message context.
In `@src/cli/render.rs`:
- Around line 439-446: The title rendering path in the Density::Narrow branch
applies truncate_with_ellipsis before sanitize_inline, which can let escaped
control characters expand past max_title_width. Update the narrow-tree rendering
logic in the branch that builds primary_parts for node.task.title to sanitize
first and then truncate, and apply the same ordering anywhere else the same
pattern appears (including the corresponding render paths in tui_render.rs and
watch.rs) to preserve the width contract.
In `@tests/skills_refresh.rs`:
- Around line 141-162: The refresh regression test currently only checks that
injected content is absent, so it can pass even if `skills refresh` fails. In
`tests/skills_refresh.rs`, update the `Command::new(tsq_bin())...output()` flow
to assert `output.status.success()` and verify the managed
`claude_skill_dir/SKILL.md` content actually changed from the pre-existing
placeholder (using the existing `refreshed` readback). This keeps the check
anchored to `resolve_managed_skill_source_directory` behavior and ensures a
trusted refresh path was used.
In `@tui-opentui/src/data.ts`:
- Around line 124-145: parseTasksEnvelope currently assumes JSON.parse always
returns a valid envelope, so it can throw on "null" and let non-array tasks
through as typed data. Update parseTasksEnvelope to mirror parseSpecEnvelope by
first verifying the parsed value is a non-null object before reading payload.ok,
payload.error, or payload.data.tasks, and return the same warning fallback when
the shape is invalid. Also harden the related parser at the other referenced
location so both envelope parsers only dereference validated objects and only
return tasks when payload.data.tasks is actually an array.
- Around line 65-78: The runTsq helper can hang indefinitely because Bun.spawn
is awaited without any timeout or abort handling, which can stall the shared
polling path. Update runTsq to use Bun’s timeout option or an AbortSignal, and
ensure the subprocess is terminated/killed on expiry before returning a
TsqSpawnResult. Keep the fix localized to runTsq and preserve the existing
stdout/stderr collection and exitCode handling.
In `@tui-opentui/src/tui-app.tsx`:
- Around line 270-274: The manual refresh handler in tui-app’s key processing is
bypassing the same concurrency protection used by the polling path, so repeated
r presses can overlap fetchTasks calls and overwrite newer state. Refactor the
refresh logic into a shared guarded function used by both the interval and the r
key branch, and ensure the guard in the refresh path prevents a new fetchTasks
call until the in-flight one finishes.
- Around line 145-171: The deps-fetch effect in TUIApp is still able to apply
stale results after the selected task or tab changes because createLatestGuard
only filters newer-issued requests, not ones that resolve after the debounce or
after leaving deps. Update the useEffect around
latestDependencyFetch/fetchDependencyTree so the async completion verifies the
current tab and selectedTask.id still match the request before calling
setDependencyRoot and setDependencyWarning, and ensure the cleanup path cancels
or invalidates any pending/debounced request when tab or selection changes.
- Around line 203-206: The stale-load guard in setSpecDialog only checks
task.id, so an older readSpecLines result can still overwrite a newer dialog for
the same task; update the check in the readSpecLines.then flow to also compare
specPath against the current dialog state. Use the specPath already stored on
the dialog state in tui-app.tsx, and make sure the stale check rejects results
when either the taskId or specPath no longer matches.
In `@tui-opentui/src/tui-views.tsx`:
- Around line 60-73: The epic rows in tui-views.tsx now render two lines, but
the pagination in tui-app.tsx still treats each epic as a single table row,
which can clip the selected epic on short terminals. Update the slicing logic
that uses tableRowBudget so it accounts for two-line rows, or reduce the epics
item budget accordingly before passing items into the table; keep the fix
aligned with the render structure in the epic row map.
---
Outside diff comments:
In `@src/cli/render.rs`:
- Around line 164-188: The CLI rendering in render_task still prints
external_ref and discovered_from without escaping, which can allow terminal
escape sequences through. Update the task field output logic in render_task to
pass both task.external_ref and task.discovered_from through sanitize_inline
before printing, matching the handling already used for assignee and
description.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed22b176-8ad1-4e4c-98fc-165f22b27f71
📒 Files selected for processing (21)
.github/workflows/ci.ymlsrc/app/service.rssrc/app/sync.rssrc/cli/render.rssrc/cli/tui_render.rssrc/cli/watch.rssrc/skills/mod.rstests/render_sanitize.rstests/skills_refresh.rstests/sync_branch.rstui-opentui/README.mdtui-opentui/package.jsontui-opentui/src/data.tstui-opentui/src/model.tstui-opentui/src/tui-app.tsxtui-opentui/src/tui-helpers.tstui-opentui/src/tui-views.tsxtui-opentui/tests/async-guards.test.tstui-opentui/tests/contract.test.tstui-opentui/tests/data.test.tstui-opentui/tests/model.test.ts
💤 Files with no reviewable changes (1)
- src/skills/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cli/render.rs`:
- Around line 52-55: The bidi-control check in is_unsafe_control is redundant at
the current sanitize() call site because sanitize() already handles bidi
characters before calling it. Remove the is_bidi_control(ch) branch from
is_unsafe_control, and keep the function focused on the remaining unsafe control
ranges so its behavior is clear if reused elsewhere.
- Around line 52-59: `is_bidi_control` in render sanitization is missing several
directional marks that should be treated as unsafe. Update the `is_bidi_control`
helper used by `is_unsafe_control` to also match U+061C, U+200E, and U+200F so
these characters are escaped like the existing bidi controls.
In `@tui-opentui/src/data.ts`:
- Around line 65-83: The runTsq function currently relies on Bun.spawn timeout
with the default kill signal, so a hung tsq subprocess may survive after timing
out. Update the Bun.spawn call inside runTsq to use SIGKILL for timeout
handling, or add equivalent manual escalation, so the spawned process cannot
ignore the timeout and outlive it.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: faf65ad4-596b-4ad8-805c-f4c2f5b07549
📒 Files selected for processing (13)
.github/workflows/ci.ymlsrc/app/sync.rssrc/cli/render.rssrc/cli/tui_render.rssrc/cli/watch.rstests/render_sanitize.rstests/skills_refresh.rstests/sync_branch.rstui-opentui/src/data.tstui-opentui/src/model.tstui-opentui/src/tui-app.tsxtui-opentui/src/tui-helpers.tstui-opentui/tests/data.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/sync.rs (1)
185-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a brief comment documenting the reordering invariant.
The data-safety property here (don't clear root events until after a
Requiredpush either succeeds or the function has already returned an error) is subtle and easy to accidentally break in a future refactor. A short comment above line 206 stating this invariant would help future maintainers avoid reintroducing data loss on required-push failure.📝 Suggested comment
+ // Only clear root events after a `Required` push has succeeded (or was + // skipped/best-effort); on `Required` push failure the `?` above returns + // early so events are preserved for a later retry. clear_repo_events(repo_root)?;🤖 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/app/sync.rs` around lines 185 - 206, Add a brief invariant comment near the end of sync::sync that documents the ordering rule: root events must not be cleared until after a PushMode::Required push succeeds, or the function has already returned an error. Point the comment at the existing control flow around git::push_current_set_upstream and clear_repo_events so future refactors preserve this data-safety guarantee.
🤖 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.
Outside diff comments:
In `@src/app/sync.rs`:
- Around line 185-206: Add a brief invariant comment near the end of sync::sync
that documents the ordering rule: root events must not be cleared until after a
PushMode::Required push succeeds, or the function has already returned an error.
Point the comment at the existing control flow around
git::push_current_set_upstream and clear_repo_events so future refactors
preserve this data-safety guarantee.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e169d2f8-ce67-4372-8ca6-44f37a3cac99
📒 Files selected for processing (7)
.github/workflows/ci.ymlsrc/app/sync.rssrc/cli/render.rstests/sync_branch.rstui-opentui/src/data.tstui-opentui/src/tui-helpers.tstui-opentui/tests/data.test.ts
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.
Integrates six reviewed fixes from an audit of the 0.7.0 release. Each landed on its own branch in an isolated worktree, passed its plan's done criteria, and was re-verified independently; this PR cherry-picks the code only (planning docs discarded). All fixes touch disjoint files and the integrated tree passes the full gate suite.
Fixes
OpenTUI repaired against the 0.7 CLI contract (
fix(tui),perf(tui))The bundled TypeScript TUI — the default
tsqTTY experience in the npm package — still calledtsq --json listanddep tree, both removed in the verb-first redesign, so every data fetch exited 1 and rendered an empty list. Rewired towatch --once/deps/spec --show; fixed the spec dialog (was reading a path against the wrong worktree root) and the epics tab (only showed the first epic); moved all subprocess calls off the render thread (asyncBun.spawn+ in-flight poll guard + debounced, stale-checked deps fetch). Added a bun test suite including a real-binary CLI contract test wired into CI, so future CLI renames fail the build instead of the shipped TUI.Skill-refresh source trust (
fix(skills))tsq skills refreshtrusted the current working directory'sSKILLS/as a source, so running it inside an untrusted repo could overwrite the user's globally installed agent skills. Dropped CWD from the implicit source-root list (explicitTSQ_SKILLS_DIR, exe-relative, and embedded sources remain); added a regression test.Terminal output sanitization (
fix(render))Task titles/descriptions/notes/spec content — attacker-influenceable and synced across machines — were printed raw, so ANSI/OSC escape sequences executed in a viewer's terminal on
tsq show/find. Added one control-character sanitizer applied at every human-print site;--jsonoutput stays byte-faithful for machine consumers.Sync-branch integrity (
fix(sync))Two independent holes in the multi-machine story: (1) the JSONL merge driver was configured only at initial setup, never on fresh clones, so a diverged sync branch fell back to a textual merge that corrupts
events.jsonl— now configured whenever the worktree is materialized or synced; (2) implicit migration pushed to origin inside read commands, sotsq find openfailed hard offline — push is now best-effort on the implicit path (warns, keeps working) while explicittsq migratestays strict, and migrated events are cleared right after the local commit.Verification
All pass on the integrated branch:
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --quietnpm run test:postinstall --prefix npm(cd tui-opentui && bun run typecheck && bun test)— 28 tests, contract tests executed against the built binaryNotes for review
tests/render_sanitize.rs,tests/skills_refresh.rs(regression),tests/sync_branch.rs(clone + offline-migration), and thetui-opentui/tests/suite.task_find.rs/plan.rs,tsq init-in-clone creating an orphan sync branch, PATH-dependent merge driver) are tracked separately, not addressed here.