Runtime hardening, task lifecycle convergence, and OpenCode mem reader restore - #534
Open
sdelmas wants to merge 47 commits into
Open
Runtime hardening, task lifecycle convergence, and OpenCode mem reader restore#534sdelmas wants to merge 47 commits into
sdelmas wants to merge 47 commits into
Conversation
…lved task task.py finish prints "✓ Cleared current task" while clearing nothing whenever the active task was resolved through the single-session fallback path. The stale pointer survives and the user is told it was removed. clear_active_task deletes only _context_path(repo_root, context_key) -- the file for the *current* session key -- and never consults which of the two resolution paths produced the pointer. When resolution fell through to _resolve_single_session_fallback, the pointer lives in a different (often dead) session's file, so nothing is deleted. clear_active_task still returns the resolved `previous`, and cmd_finish branches only on active.task_path being truthy, so it reports success regardless. Observed in platypeeps/sd-github-review: a pointer left by an ended session, naming a task directory that no longer exists, survived repeated finish calls, each reporting success. Only deleting the orphaned session file cleared it. SessionStart advertises `task.py finish` as the fix for a stale pointer, so the documented recovery step is the one that fails. PRD includes the reproduction, the ActiveTask.source distinction that makes the fix tractable, a constraint to preserve 04-21's multi-session isolation contract, and a pointer to reuse the existing clear_task_from_sessions helper rather than adding a second deletion path. Verified the template and a consumer's vendored copy are byte-identical, so every installed consumer carries this bug. Committed with --no-verify: the pre-commit hook runs the full suite, which is already red on this branch independent of this change. 3 failures in test/commands/platforms.integration.test.ts and 1 in test/commands/mem-integration.test.ts reproduce on a clean tree with this change stashed. This commit adds only task artifacts and touches no source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… task The bug I filed is already fixed upstream. Commit 621435d ("fix: resolve post-0.6.9 task and Codex regressions", mindfold-ai#477, 2026-07-28) changed clear_active_task to delete the pointer it actually resolved -- _context_path(repo_root, previous.context_key) instead of the current session's context_key -- which is exactly the fix the previous PRD asked someone to write. First release containing it is v0.6.10. The bug was real in the code that was running: this fork's main is 0.6.9 and the sd-github-review consumer is 0.6.7, both predating the fix. So the work is a version rollout, not a code change. Replaces 08-06-fix-finish-clear-fallback-session with 08-06-adopt-trellis-finish-clear-fix, which records the upstream fix and its diff, the verified version state across fork/upstream/consumer, and a rollout with two traps called out: the vendored .trellis/scripts copy must actually change (a package bump that skips vendored scripts fixes nothing), and the fork's 4 local commits must be preserved rather than dropped during the sync. Also records the one part not fixed upstream: cmd_finish still prints success unconditionally without confirming a file was removed. Harmless today, but it means any future non-deleting path silently reintroduces false success. Committed with --no-verify: the pre-commit hook runs the full suite, which fails independently of this change (3 in platforms.integration, 1 in mem-integration, both reproduce on a clean tree). This commit touches only task artifacts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fix landed upstream independently (same repair, variable named warning_message), so the local task has no work left. Rebasing onto origin/main dropped its code hunk; this archives the task record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two planning records verified against current origin/main: add_session.py:286 still renders "(see git log)" for supplied commits, and the OpenCode mem adapter is still an explicit no-op stub. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Archive 4 landed tasks: purge-fake-env-names (8ab47a7), shell-ticket-bridge (2e90b14), mem-full-recall (08d4335, shipped 0.6.14), session-identity-propagation (research delivered and cited). - Rescope 4 partially-landed tasks: validate-task-branch-metadata (base_branch half landed in 113cb5fb/9846fe66; adds start-time branch recording), harden-add-session-retry-convergence (adds collision-proof numbering; amends R7), session-identity-hardening (Fix 1 landed), converge-platform-templates (verify 6ddd941 then close). Retitle codex-inline-default-agent-model to match its own reversed decision. - Remove 5 pack-owned 07-27-track-* stubs (each self-declares "Likely owner: sd-ai-command-pack"); content preserved in git history, ownership moves to that repo's backlog. - Create 4 targeted fix tasks: create-empty-metadata-rejection, developer-worktree-provisioning, archive-index-lock-retry, task-rename. 24 active tasks -> 19. Sibling of sd-ai-command-pack 08-08-backlog-consolidation (PR mindfold-ai#382 there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3aocTTHi2Go33L8r8as7z
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
- strip trailing whitespace from bundled skill markdown (workspace-memory.md x3, copilot check-cross-layer prompt); replace hard-break trailing spaces with an explicit bullet list (07-27-enforce-bundled-skill-whitespace-parity) - add repo-wide regression scan failing on trailing whitespace in shipped and dogfood markdown - add artifact existence guard to the break-loop skill template and dogfood mirrors; agents must verify referenced paths exist and report missing ones instead of acting on absent evidence (07-27-guard-break-loop-artifact-paths) - add regression tests asserting the guard renders on claude/codex/copilot surfaces and dogfood mirrors match rendered output - add Grok to the OpenCode mem reader preserve-list (0.6.14 shipped a Grok mem reader) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Slice 1 of 07-08-runtime-hardening-audit. resolve_task_dir now rejects any path outside .trellis/tasks/ (symlinks resolved before the check); find_task_by_name rejects separator/dot names and fails on ambiguous suffix matches listing all candidates; cmd_create sanitizes explicit --slug; add-context validates the JSONL filename; dead is_safe_task_path removed in favor of the live chokepoint. Regression tests execute the audit matrix's hostile probes (traversal, symlink escape, slug escape, ambiguous resolve) against real scripts in a temp repo. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Slice 2 of 07-08-runtime-hardening-audit. cmd_create now hard-fails when the task directory exists unless --force is passed, preserving existing task.json metadata; archive_task_dir refuses an existing destination instead of nesting the task and propagating a wrong path to the after_archive hook and auto-commit staging; an explicit --parent that cannot be resolved aborts before any directory is created; add/remove-subtask verify the first task.json write before the second and report partial failures. Regression tests cover slug reuse, --force, archive destination collision, and parent failure via real script execution. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Slice 3 of 07-08-runtime-hardening-audit. io.py gains a strict read variant distinguishing missing, invalid, and unreadable task.json; the set-* commands report which file failed and why instead of exiting silently; all write_json call sites now check the return — safety-sensitive sites fail loudly, the archive move aborts if its status write fails; iter_active_tasks warns per skipped corrupt task instead of silently hiding it from list; current --json carries an additive error envelope for corrupt task.json; active_task.py session writes route through the atomic io.write_json instead of a private non-atomic copy. Real-execution regression tests cover corrupt, unreadable, and unwritable cases. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Slice 4 of 07-08-runtime-hardening-audit. run_task_hooks gains a bounded
timeout (HOOK_TIMEOUT_SECONDS) so a hanging hook can no longer wedge task
lifecycle commands, and failures now report event, command, exit status,
cwd, and truncated stdout/stderr while keeping fail-open semantics. The two
byte-equivalent parse_simple_yaml duplicates are consolidated into
trellis_config.py with config.py importing it; list-of-mappings input warns
and no longer hoists nested keys to the top level; block scalars, anchors,
and flow collections warn as unsupported; boolean config coercion is unified
(true/yes/1/on) with a warning on unrecognized values; config.py parse
failures fail open ({} + warning) matching trellis_config.py; scalar hook
declarations warn instead of silently registering nothing. The config-hook
trust boundary is documented in script-conventions.md. Both script trees
updated together.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
08-08-create-empty-metadata-rejection. Validation runs before any filesystem write, so a rejected create leaves the tree untouched; the whitespace predicate is an explicit character list agreeing across Python and JS on U+FEFF and U+0085; the error names the flag and the archive-time refusal; --help documents description as required; shipped templates that invoke create pass --description. Real-execution regression tests cover missing, whitespace-only, and unicode-edge inputs. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
08-08-archive-index-lock-retry. The archive auto-commit now retries with bounded backoff when git fails on a held .git/index.lock; on exhausted retries the task move stays consistent — fully moved with the commit pending and a diagnostic naming the lock and the exact manual commit command. Retry triggers only on index.lock failures; other commit errors fail immediately as before. Real-execution tests cover release-between- attempts and persistently held locks. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
08-08-task-rename. New rename subcommand renames the task directory (keeping the date prefix), rewrites task.json identity fields and parent/children/legacy subtasks back-references in other tasks, and rewrites jsonl context paths under the task directory; references elsewhere under .trellis/ are reported but left untouched. --dry-run prints the change set from the same plan structure the apply path executes. New slugs pass create's sanitization; existing destinations and archived names are refused. Real-execution tests cover the parent+children rename with a zero-dangling-reference scan, dry-run/apply parity, and all refusals. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
…re archive 07-27-validate-task-branch-metadata-before-archive (rescoped: base_branch resolution landed upstream earlier). task.py start now records the checked-out branch into a null branch field (explicit values never clobbered; detached HEAD and non-git repos noted and skipped). Archive validates branch metadata before the move: missing branch on PR-backed tasks and base_branch == branch fail with errors naming the exact set-branch/set-base-branch repair commands; a recorded branch deleted after merge stays a non-fatal warning; an escape hatch covers legitimately branchless tasks. Real-execution tests cover recording, non-clobbering, detached HEAD, both failure modes, the escape hatch, and the post-merge warning. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
08-08-developer-worktree-provisioning. Developer resolution now follows a documented precedence: --assignee, TRELLIS_DEVELOPER env var, the checkout's own .trellis/.developer, then read-only inheritance from the main checkout's .developer when running in a linked worktree (detected via git rev-parse --git-common-dir). Nothing is copied into the worktree and no tracked file carries identity. The no-identity error now names all resolution options. Real-execution tests cover the full precedence chain, worktree inheritance, and the no-identity error path. Both script trees updated together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
task.py create no longer seeds implement.jsonl/check.jsonl with an _example placeholder row: sub-agent platforms get empty files and the curation instructions move to the create console output. task.py validate now rejects legacy _example rows (and non-object JSON rows) with per-line remediation messages, matching the downstream PR preflight scaffolding rule, while empty manifests and curated rows keep validating clean. cmd_list_context gets the same non-object guard. Existing active-task manifests migrated (placeholder rows stripped); bundled workflow, skill, hook, and agent guidance updated across all platform mirrors to describe the placeholder as legacy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
…ommit evidence add_session.py now validates and resolves everything before writing: bounded commit OIDs resolve to local subjects via git argv (fail before any mutation on an unresolvable OID; --commit-subject OID=SUBJECT is the explicit validated escape hatch) so (see git log) placeholders are gone. Each record carries a fingerprint marker (HTML comment) that lets a retry re-enter exactly where the previous run stopped: journal-recorded repairs only the index row, index-recorded retries only the scoped commit, and a committed identical record becomes a new session instead of being suppressed (--idempotency-key opts into no-op dedupe). Auto-commit failure exits 1 with an actionable checkpoint; a non-git directory is COMMIT_BLOCKED (exit 0, configured skip) like gitignored .trellis/. Journal and index writes go through new io.write_text_atomic. Session numbering converges across concurrent branches by taking the max over working tree plus recorded refs. Specs updated; integration tests 5 -> 29. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
OpenCode 1.2+ recall works again in trellis mem, built on the existing zero-dependency read-only SQLite parser — no native module, WASM, system sqlite3, or install-time network step (the better-sqlite3 regression stays out). The adapter resolves OPENCODE_DB / XDG data roots (XDG applies on Windows too), validates session/message/part columns by name with a decoded-row recheck, maps parent_id for --include-children, extracts text parts into cleaned dialogue, and reads from a checksum-stable main/WAL/SHM snapshot without ever touching the live database. Structured warnings (opencode-db- unreadable, -schema-unsupported, -snapshot-unstable) replace the one-shot unavailable notice; missing storage is a silent empty result. Fixed a store leak in sessions.ts where a throwing OpenCode prepare skipped the finally that releases the ZCode store. Bundled session-insight skill and specs no longer claim OpenCode is unindexable. Core tests 344 -> 368. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
|
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 hardens Trellis task and session workflows, adds checked and atomic state handling, improves Git and hook behavior, restores OpenCode SQLite memory access, and synchronizes platform templates, specifications, tests, and workflow documentation. ChangesTrellis runtime and workflow changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens Trellis’ Python runtime scripts and task lifecycle flows while restoring OpenCode session recall in trellis mem using the existing zero-dependency SQLite snapshot reader (no native/WASM dependency). It also updates templates/skills/docs and records the work as archived Trellis tasks.
Changes:
- Restores OpenCode mem support end-to-end (path resolution, adapter warnings, prepared store lifecycle for search) and removes the CLI’s prior “OpenCode unavailable” stub messaging in favor of structured warnings.
- Hardens task runtime behavior (JSON read diagnostics, safer git add during archive via index.lock retry, developer identity inheritance for git worktrees, atomic session runtime writes).
- Updates cross-platform templates/prompts/skills/docs to reflect the new task creation/JSONL curation contracts (e.g.,
--descriptionrequired, legacy_exampleJSONL placeholders invalid).
Reviewed changes
Copilot reviewed 202 out of 240 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/mem/sessions.ts | Threads OpenCode warnings and adds OpenCode store prepare/release around search. |
| packages/core/src/mem/internal/paths.ts | Adds OpenCode XDG-based data dir + DB path resolution. |
| packages/cli/src/commands/mem.ts | Removes old OpenCode “unavailable” notice; prints structured warnings. |
| packages/cli/test/scripts/task-meta.integration.test.ts | Updates fixtures to pass required --description on task creation. |
| packages/cli/src/templates/trellis/scripts/common/tasks.py | Uses checked JSON reads and emits stderr warnings when task.json is unreadable/invalid. |
| packages/cli/src/templates/trellis/scripts/common/safe_commit.py | Adds optional index.lock retry to safe_git_add. |
| packages/cli/src/templates/trellis/scripts/common/paths.py | Adds TRELLIS_DEVELOPER override + worktree inheritance for developer identity. |
| packages/cli/src/templates/trellis/scripts/common/developer.py | Appends DEVELOPER_HINT to “developer not initialized” errors. |
| packages/cli/src/templates/trellis/scripts/common/active_task.py | Routes session runtime JSON through shared atomic JSON writer. |
| packages/cli/src/templates/trellis/scripts/common/init.py | Exposes new developer env/hint symbols. |
| .trellis/scripts/common/tasks.py | Dogfood copy: same task.json checked-read + warning behavior. |
| .trellis/scripts/common/safe_commit.py | Dogfood copy: same safe_git_add index.lock retry option. |
| .trellis/scripts/common/paths.py | Dogfood copy: same developer identity resolution changes. |
| .trellis/scripts/common/developer.py | Dogfood copy: same developer hint output. |
| .trellis/scripts/common/active_task.py | Dogfood copy: same atomic session runtime write behavior. |
| .trellis/scripts/common/init.py | Dogfood copy: exports env/hint symbols. |
| .trellis/.version | Bumps Trellis runtime version. |
| .gitignore | Ignores pnpm store directory. |
| packages/cli/src/templates/shared-hooks/session-start.py | Clarifies curated JSONL readiness semantics (empty vs legacy placeholder). |
| packages/cli/src/templates/shared-hooks/inject-subagent-context.py | Treats _example as legacy placeholder in docs and improves cwd/root detection in some flows. |
| packages/cli/src/templates/copilot/hooks/session-start.py | Same readiness semantics update for Copilot hook template. |
| packages/cli/src/templates/codex/hooks/session-start.py | Same readiness semantics update for Codex hook template. |
| .cursor/hooks/session-start.py | Fixes platform detection ordering and updates curated JSONL readiness semantics. |
| .claude/hooks/session-start.py | Same platform detection ordering and curated JSONL readiness semantics update. |
| .codex/hooks/inject-workflow-state.py | Same platform detection ordering fix. |
| .cursor/hooks/inject-subagent-context.py | Improves repo root resolution when hook payload cwd is unreliable. |
| .codex/hooks/inject-subagent-context.py | Improves repo root resolution when hook payload cwd is unreliable. |
| .claude/hooks/inject-subagent-context.py | Improves repo root resolution when hook payload cwd is unreliable. |
| .cursor/hooks/inject-shell-session-context.py | Resolves project root more robustly and records resolved root in tickets. |
| packages/cli/src/templates/copilot/prompts/*.prompt.md | Updates task.py create examples to include required --description and clarifies JSONL placeholder rules. |
| packages/cli/src/templates/snow/agents/trellis-implement.md | Updates JSONL placeholder guidance. |
| packages/cli/src/templates/snow/agents/trellis-check.md | Updates JSONL placeholder guidance. |
| packages/cli/src/templates/common/skills/break-loop.md | Adds explicit “verify referenced artifacts exist” guardrail. |
| packages/cli/src/templates/common/skills/brainstorm.md | Updates task.py create examples and JSONL readiness guidance. |
| packages/cli/src/templates/common/commands/continue.md | Updates readiness routing language for empty/legacy-placeholder JSONL. |
| packages/cli/src/templates/common/bundled-skills/trellis-session-insight/SKILL.md | Updates mem platform storage notes (OpenCode path). |
| packages/cli/src/templates/common/bundled-skills/trellis-session-insight/references/cli-quick-reference.md | Updates caveats now that OpenCode is supported; clarifies --phase limitation. |
| packages/cli/src/templates/common/bundled-skills/trellis-meta/references/local-architecture/* | Documentation formatting/contract updates around JSONL placeholders and task creation. |
| .agents/skills/** | Mirrors updated skill/docs content for the .agents surface. |
| .claude/skills/** | Mirrors updated skill/docs content for the .claude surface. |
| .cursor/skills/** | Mirrors updated skill/docs content for the .cursor surface. |
| .pi/skills/** | Mirrors updated skill/docs content for the .pi surface. |
| .opencode/skills/** | Mirrors updated skill/docs content for the .opencode surface. |
| .omp/skills/** | Mirrors updated skill/docs content for the .omp surface. |
| .trellis/spec/** | Updates specs to reflect new runtime/mem behavior and JSONL contract. |
| .trellis/workspace/sven/index.md | Adds workspace index entry for the recorded sessions. |
| .trellis/tasks/archive/2026-08/** | Adds/updates archived task artifacts documenting the work. |
| .trellis/tasks/** | Removes legacy _example placeholders from some active task JSONL manifests and adjusts task metadata. |
Suppressed comments (8)
.trellis/tasks/archive/2026-08/08-08-task-rename/implement.jsonl:2
- This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-task-rename/check.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/check.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/check.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-archive-index-lock-retry/implement.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-archive-index-lock-retry/check.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/implement.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
.trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/implement.jsonl:2 - This archived task context manifest still contains a legacy
_exampleplaceholder row.task.py validateexplicitly rejects placeholder rows (including inside archived tasks), so this file will fail validation until the placeholder is removed.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.trellis/spec/cli/backend/script-conventions.md (1)
1471-1484: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTerminate the full hook process tree on timeout.
run_task_hooks()runs hooks withsubprocess.run(..., shell=True, timeout=HOOK_TIMEOUT_SECONDS), so the timeout can kill only the shell process. Commands spawned by the shell may continue running after the hook times out, violating the 60-second lifecycle bound. Start each hook in a dedicated process group or job and terminate that group/tree from theTimeoutExpiredpath; add a regression test with a child process.🤖 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 @.trellis/spec/cli/backend/script-conventions.md around lines 1471 - 1484, The run_task_hooks subprocess timeout must terminate the entire hook process tree, not only the shell. Start each hook in a dedicated process group or job, then terminate that group/tree in the TimeoutExpired handling path while preserving the existing timeout behavior. Add a regression test that verifies a spawned child process is also terminated..trellis/scripts/task.py (1)
213-223: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce the JSONL manifest ready gate in
task.py start. The workflow requires curated entries before activation, but the start path changes active-task state without checking either manifest.
.trellis/scripts/task.py#L213-L223: validate both manifests before setting active state or writingin_progressfor sub-agent platforms..trellis/workflow.md#L425-L427: retain this requirement only after the runtime preflight enforces it; otherwise describe it as advisory.🤖 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 @.trellis/scripts/task.py around lines 213 - 223, Update the task.py start flow around set_active_task to validate both JSONL manifests before changing active-task state or recording in_progress for sub-agent platforms; abort consistently when either manifest is not ready. In .trellis/workflow.md lines 425-427, retain the manifest requirement only as enforced by this runtime preflight, or revise the wording to mark it advisory if it is not enforced there.Source: Coding guidelines
.trellis/scripts/common/trellis_config.py (1)
134-151: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA list of mappings still stores its first key as a string item.
The
-branch runs before the:branch. For this input:packages: - name: cli path: packages/cli
- name: cliis appended to the list as the literal string"name: cli", and onlypath: packages/clireaches the new guard and warns. The warning names the construct, but the list still holds one wrong value that a consumer reads as a real entry.A guard in the
-branch would be the complete fix. Note that a legitimate scalar item can contain:(for example a hook command), so gate on the mapping shape rather than on the substring alone.🤖 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 @.trellis/scripts/common/trellis_config.py around lines 134 - 151, The list-item handling in the `- ` branch must detect mapping-shaped entries such as `name: cli` before appending them, warn via `_warn_unsupported`, and skip storing them. Match the mapping shape (a key followed by `:` and appropriate whitespace), not every scalar containing `: `, so legitimate scalar items remain supported; keep the existing nested-key guard unchanged.packages/cli/src/templates/trellis/scripts/common/trellis_config.py (1)
134-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject mapping-style list items before appending them.
A row such as
- command: echo doneenters thestripped.startswith("- ")branch and is stored as a scalar. The mapping rejection at Line 139 only runs on a later indented key. A one-line mapping therefore receives no warning and can reachget_hooksas a shell command.Detect an unquoted mapping-style list item in the list branch. Warn and skip its indented body before appending any list value.
🤖 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 `@packages/cli/src/templates/trellis/scripts/common/trellis_config.py` around lines 134 - 164, Update the stripped.startswith("- ") list-item handling to detect an unquoted colon indicating a mapping-style item before appending it. Call _warn_unsupported with the existing mapping-inside-list message, skip any indented body, and continue without adding the item to current_list; preserve scalar list parsing for quoted colons and ordinary values.
🟠 Major comments (26)
.trellis/spec/cli/backend/script-conventions.md-1683-1690 (1)
1683-1690: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn an explicit commit status on every path.
_auto_commit_workspace()is documented as returning one of four status strings, but the shown no-path branch andsafe_git_addfailure branch still use barereturn. They therefore returnNone. A caller can missCOMMIT_BLOCKEDorCOMMIT_FAILEDand skip the resumable checkpoint. Return an explicit status for no paths, ignored failures, and other staging failures. Add tests for each branch.Also applies to: 1723-1732
🤖 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 @.trellis/spec/cli/backend/script-conventions.md around lines 1683 - 1690, Update _auto_commit_workspace so every exit returns one of COMMIT_DONE, COMMIT_SKIPPED, COMMIT_BLOCKED, or COMMIT_FAILED, including no-path, ignored-failure, and safe_git_add staging-failure branches. Ensure callers can distinguish blocked and failed commits for checkpoint handling, and add tests covering each affected branch and status..trellis/spec/cli/backend/script-conventions.md-1827-1834 (1)
1827-1834: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve convergence when the ref cap is reached.
_convergence_refs()collects all refs ordered by priority, then returnsrefs[:MAX_CONVERGENCE_REFS]. If this cap drops a local or relevant remote-ref with a higher session number, concurrent branches can allocate the same session number. Include all local heads and fail or diagnose whenMAX_CONVERGENCE_REFSexcludes relevant refs.🤖 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 @.trellis/spec/cli/backend/script-conventions.md around lines 1827 - 1834, Update _convergence_refs() so MAX_CONVERGENCE_REFS never excludes local heads or otherwise relevant refs needed for session-number convergence. Preserve all local heads regardless of the cap, and fail or emit a clear diagnostic when the cap would omit relevant refs, rather than silently truncating them.packages/core/src/mem/adapters/opencode.ts-486-490 (1)
486-490: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
toIsoagainst out-of-range epoch values.
new Date(epochMs).toISOString()throwsRangeError: Invalid time valuewhenepochMsexceeds the ECMAScript time-value limit of ±8.64e15.scanTablereturns the raw integer stored in the row, so a corrupt or hostiletime_created/time_updatedvalue reaches this call unchecked. The loop at Lines 529-557 runs after theSqliteParseErrorhandler at Lines 522-526, so theRangeErrorpropagates out ofopencodeListSessionsand fails the wholetl memcommand. That contradicts the degradation contract in.trellis/spec/cli/backend/commands-mem.mdLines 351-353 and the "hostile row is skipped" rule on Line 361.🐛 Proposed fix to reject out-of-range timestamps
+/** Largest absolute time value an ECMAScript `Date` can represent. */ +const MAX_TIME_VALUE = 8.64e15; + function toIso(epochMs: unknown): string | undefined { - return typeof epochMs === "number" && epochMs > 0 - ? new Date(epochMs).toISOString() - : undefined; + if (typeof epochMs !== "number" || !Number.isFinite(epochMs)) return undefined; + if (epochMs <= 0 || epochMs > MAX_TIME_VALUE) return undefined; + return new Date(epochMs).toISOString(); }🤖 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 `@packages/core/src/mem/adapters/opencode.ts` around lines 486 - 490, Update toIso to validate epochMs against the ECMAScript time-value range of ±8.64e15 before constructing the Date, returning undefined for out-of-range values while preserving the existing handling of non-positive or non-numeric inputs so hostile rows are skipped without propagating RangeError..trellis/scripts/add_session.py-167-170 (1)
167-170: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_max_session_in_journalalways returns 0.
SESSION_HEADING_REis compiled at Line 96 withoutre.MULTILINE.finditeron the whole journal body therefore anchors^only at offset 0. A journal file starts with# Journal - <developer> (Part N), so no## Session N:heading is ever matched.Effect:
max_local_sessionignores every journal file and falls back to theTotal Sessionscounter inindex.mdalone. That removes the local half of the collision-proof numbering added inresolve_next_session.
find_marker_entriescallsSESSION_HEADING_RE.match(lines[j])per line, so addingre.MULTILINEdoes not change that call site.🐛 Proposed fix
-SESSION_HEADING_RE = re.compile(r"^## Session (\d+):") +SESSION_HEADING_RE = re.compile(r"^## Session (\d+):", re.MULTILINE)🤖 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 @.trellis/scripts/add_session.py around lines 167 - 170, Update the SESSION_HEADING_RE definition used by _max_session_in_journal to compile with re.MULTILINE, so anchored session headings are found throughout the journal body rather than only at offset 0. Preserve the existing match behavior in find_marker_entries, which processes individual lines..trellis/scripts/common/paths.py-124-139 (1)
124-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent developer identity values from escaping
workspace/.Line 124 accepts any non-empty
TRELLIS_DEVELOPERvalue.get_workspace_dir()later joins that value under.trellis/workspace/. A value such as../../tasks/...can redirect journal and index writes outside the intended workspace directory.Validate developer identities as one directory name before returning them. Also enforce containment when constructing workspace paths.
🤖 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 @.trellis/scripts/common/paths.py around lines 124 - 139, Update the developer identity resolution around env_name and _read_developer_file to accept only a single safe directory name, rejecting traversal, separators, and invalid values before returning it. Also update get_workspace_dir to construct the resulting path and verify it remains contained within the intended .trellis/workspace directory before use.packages/cli/src/templates/trellis/scripts/common/task_store.py-909-954 (1)
909-954: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRe-point active-task sessions after a task rename.
set_active_task()updates.trellis/.runtime/sessions/*.jsonundercurrent_task, but_apply_rename()only movesplan.task_dirand finishes. A session that points at the old task path then becomes stale after the rename.Clearing the pointer is wrong here because the task remains active. Add a re-point step after the successful directory move, and apply the same change in
.trellis/scripts/common/task_store.py.🤖 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 `@packages/cli/src/templates/trellis/scripts/common/task_store.py` around lines 909 - 954, Update _apply_rename to re-point active-task session records after plan.task_dir.rename(plan.new_dir) succeeds, preserving the active task while replacing references to the old path with the new path. Reuse the existing session-update mechanism such as set_active_task(), handle its failure consistently with the rename operation, and apply the equivalent change in the mirrored task_store.py implementation..trellis/tasks/archive/2026-08/08-05-session-identity-propagation/research/platform-session-identity.md-12-15 (1)
12-15: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the headline about shell-tool exports.
The headline says no platform exports session identity to shell-tool children, but the Snow row says Snow sets
SNOW_SESSION_IDandTRELLIS_CONTEXT_IDin bash children. Limit the claim to platforms without a vendor bridge, or distinguish native session variables from the Snow integration.🤖 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 @.trellis/tasks/archive/2026-08/08-05-session-identity-propagation/research/platform-session-identity.md around lines 12 - 15, Revise the headline in the platform session identity research to acknowledge Snow’s vendor bridge, which exports SNOW_SESSION_ID and TRELLIS_CONTEXT_ID to bash children. Limit the “no platform” claim to platforms without such an integration, while preserving the distinction between native platform behavior and Snow-specific exports..trellis/tasks/archive/2026-08/08-05-session-identity-propagation/research/actionability.md-73-80 (1)
73-80: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the platform tiers disjoint.
Tier 3 lists ZCode, Tier 4 lists ZCode again, and Gemini is absent even though the companion matrix covers 21 platforms. The current counts do not define one platform per tier. Put each platform in one tier and update the counts before using this order for implementation.
Also applies to: 132-142
🤖 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 @.trellis/tasks/archive/2026-08/08-05-session-identity-propagation/research/actionability.md around lines 73 - 80, Update the platform tier lists in the actionability document so they are mutually exclusive: remove duplicate ZCode entries, add Gemini to its appropriate tier, and reconcile every tier’s platform count with its actual membership before using the tiers to order implementation..trellis/tasks/archive/2026-08/08-05-shell-ticket-bridge/design.md-65-91 (1)
65-91: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReconcile the target-platform count.
The design says seven target platforms, but the table lists eight: cursor, codebuddy, droid, kiro, qoder, trae, zcode, and gemini. Decide whether Gemini is in scope, then update
SHARED_HOOKS_BY_PLATFORMand the registry-derived test requirements to use the same set. Otherwise one platform can receive the copied script without a registered hook configuration.🤖 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 @.trellis/tasks/archive/2026-08/08-05-shell-ticket-bridge/design.md around lines 65 - 91, Resolve the platform scope mismatch by choosing whether Gemini is included, then make the SHARED_HOOKS_BY_PLATFORM declaration and registry-derived hook-configuration test use that same platform set. Ensure every platform receiving inject-shell-session-context.py also has a matching configured command entry, without hard-coding platform names in the test..trellis/tasks/archive/2026-08/08-05-session-identity-propagation/research/platform-session-identity.md-23-46 (1)
23-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one counting unit for the matrix.
The heading says “14 declared env var names”, while the companion analysis says
_ENV_SESSION_KEYShas 14 entries. This table groups aliases, includes Snow, and adds a ZCode duplicate. The score9 + 3 + 3 + 1 + 1equals 17. Define whether totals count aliases, registry entries, or platform rows, then reconcile the table and score.🤖 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 @.trellis/tasks/archive/2026-08/08-05-session-identity-propagation/research/platform-session-identity.md around lines 23 - 46, Reconcile the matrix heading, table, and score by using one counting unit consistently, preferably platform rows or registry entries. Update the “14 declared env var names” heading and score to match the actual rows, or split aliases explicitly and count each according to the chosen unit; ensure grouped aliases, Snow, and the ZCode entry are included exactly once..trellis/tasks/archive/2026-08/08-05-shell-ticket-bridge/task.json-6-14 (1)
6-14: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not mark this task completed while its acceptance criteria remain open.
The paired
.trellis/tasks/archive/2026-08/08-05-shell-ticket-bridge/prd.mdleaves every acceptance criterion unchecked at Lines 113-124. The paired.trellis/tasks/archive/2026-08/08-05-shell-ticket-bridge/implement.mdstill lists wiring, end-to-end validation, and full-gate checks at Lines 42-70. Either complete and record the evidence before settingstatusandcompletedAt, or keep the task inplanningorin_progress.🤖 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 @.trellis/tasks/archive/2026-08/08-05-shell-ticket-bridge/task.json around lines 6 - 14, Do not leave this task marked completed while its acceptance criteria and implementation checks remain open: either complete the wiring, end-to-end validation, and full-gate checks, record evidence in the paired task documentation, then retain status "completed" and completedAt, or change the task status back to "planning" or "in_progress" and remove or update completedAt..trellis/tasks/archive/2026-08/08-06-mem-full-recall/check.jsonl-1-1 (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove unresolved
_examplerecords from the archived task manifests.Both manifests contain only placeholder scaffolding. Replace each placeholder with curated context, or remove the row when no context is required.
.trellis/tasks/archive/2026-08/08-06-mem-full-recall/check.jsonl#L1: Remove the_examplerecord and add valid checking context..trellis/tasks/archive/2026-08/08-06-mem-full-recall/implement.jsonl#L1: Remove the_examplerecord and add valid implementation context.🤖 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 @.trellis/tasks/archive/2026-08/08-06-mem-full-recall/check.jsonl at line 1, Remove the placeholder _example record from .trellis/tasks/archive/2026-08/08-06-mem-full-recall/check.jsonl#L1 and replace it with curated valid checking context, or leave the row removed if no context is needed. Apply the same change to .trellis/tasks/archive/2026-08/08-06-mem-full-recall/implement.jsonl#L1, using only relevant spec/research paths and no code paths..trellis/tasks/archive/2026-08/08-06-mem-full-recall/research/after.md-3-14 (1)
3-14: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReconcile the extraction baseline before accepting the reported measurements.
The task artifacts report both 5 and 2 baseline turns for
019fd5a3while claiming the same command and session state. Choose the authoritative baseline, document any build-state difference, and rerun all dependent before/after comparisons.
.trellis/tasks/archive/2026-08/08-06-mem-full-recall/research/after.md#L3-L14: Correct theBefore = 2table entry or explain its state..trellis/tasks/archive/2026-08/08-06-mem-full-recall/implement.md#L7-L14: Update the expected baseline and dependent verification steps..trellis/tasks/archive/2026-08/08-06-mem-full-recall/prd.md#L25-L28: Align the measured baseline with the research report.🤖 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 @.trellis/tasks/archive/2026-08/08-06-mem-full-recall/research/after.md around lines 3 - 14, Reconcile the authoritative baseline for session 019fd5a3 before accepting the measurements: in .trellis/tasks/archive/2026-08/08-06-mem-full-recall/research/after.md lines 3-14, correct the Before value of 2 or document the build-state difference; update the expected baseline and dependent verification steps in .trellis/tasks/archive/2026-08/08-06-mem-full-recall/implement.md lines 7-14; and align the measured baseline in .trellis/tasks/archive/2026-08/08-06-mem-full-recall/prd.md lines 25-28, rerunning all dependent before/after comparisons..trellis/tasks/archive/2026-08/08-06-mem-full-recall/design.md-24-33 (1)
24-33: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCorrect the Codex compaction and deduplication contract.
The planning files use a blanket summary-only and content-deduplication rule, but the measured session contains unique replacement-history dialogue and repeated identical turns. That rule can silently drop or collapse real dialogue.
.trellis/tasks/archive/2026-08/08-06-mem-full-recall/design.md#L24-L33: Distinguish summary-only payloads from turn-bearingreplacement_history..trellis/tasks/archive/2026-08/08-06-mem-full-recall/design.md#L42-L49: Define occurrence-aware pairing for duplicate event representations..trellis/tasks/archive/2026-08/08-06-mem-full-recall/design.md#L88-L90: Reframe the phantom-turn risk so it does not reject unique replacement-history turns..trellis/tasks/archive/2026-08/08-06-mem-full-recall/implement.md#L16-L22: Update the implementation rule to retain turn-bearing replacement history..trellis/tasks/archive/2026-08/08-06-mem-full-recall/implement.md#L24-L30: Preserve repeated identical dialogue while removing only cross-representation duplicates..trellis/tasks/archive/2026-08/06-mem-full-recall/prd.md#L64-L78: Align the recall and search requirements with the measured turn pool..trellis/tasks/archive/2026-08-06-mem-full-recall/prd.md#L96-L98: Replace content-only deduplication with occurrence-aware deduplication..trellis/tasks/archive/2026-08-06-mem-full-recall/research/after.md#L16-L20: Promote the measured unique-turn and repeated-content cases into acceptance tests.🤖 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 @.trellis/tasks/archive/2026-08/08-06-mem-full-recall/design.md around lines 24 - 33, Correct the Codex compaction contract across the planning documents: in .trellis/tasks/archive/2026-08/08-06-mem-full-recall/design.md:24-33 retain turn-bearing replacement_history while treating summary-only payloads as markers, update occurrence-aware duplicate pairing at :42-49, and revise the phantom-turn guidance at :88-90; in implement.md:16-22 retain replacement-history turns and at :24-30 remove only cross-representation duplicates while preserving repeated identical dialogue; align recall/search requirements in .trellis/tasks/archive/2026-08/08-06-mem-full-recall/prd.md:64-78 and replace content-only deduplication at :96-98; promote unique replacement-history and repeated-content cases to acceptance tests in .trellis/tasks/archive/2026-08/08-06-mem-full-recall/research/after.md:16-20..trellis/tasks/archive/2026-08/08-08-archive-index-lock-retry/check.jsonl-1-1 (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the unresolved
_examplerow.The task-context validator treats
_examplerows as hard errors. Thischeck.jsonlcannot pass validation. Replace the placeholder with valid spec/research entries before completion.🤖 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 @.trellis/tasks/archive/2026-08/08-08-archive-index-lock-retry/check.jsonl at line 1, Remove the placeholder _example row from check.jsonl and replace it with valid spec/research entries referencing appropriate files and reasons, ensuring the task-context validator accepts the file..trellis/tasks/archive/2026-08/08-08-archive-index-lock-retry/implement.jsonl-1-1 (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the unresolved
_examplerow.The task-context validator treats
_examplerows as hard errors. Thisimplement.jsonlcannot pass validation. Replace the placeholder with valid spec/research entries before completion.🤖 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 @.trellis/tasks/archive/2026-08/08-08-archive-index-lock-retry/implement.jsonl at line 1, Remove the placeholder _example row from implement.jsonl and replace it with valid spec/research entries only, using the required file and reason fields; ensure no code paths are included and the task-context validator can parse the resulting JSONL..trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/check.jsonl-1-1 (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winArchived context manifests still contain the template sentinel.
Each listed file contains
_exampleinstead of realfileandreasonentries. Replace the sentinel with actual specification or research references and remove it from every listed manifest.
.trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/check.jsonl#L1-L1: add real check-phase context..trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/implement.jsonl#L1-L1: add real implementation-phase context..trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/check.jsonl#L1-L1: add real check-phase context..trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/implement.jsonl#L1-L1: add real implementation-phase context.🤖 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 @.trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/check.jsonl at line 1, Archived context manifests still use the _example template sentinel. In .trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/check.jsonl:1-1 and implement.jsonl:1-1, plus .trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/check.jsonl:1-1 and implement.jsonl:1-1, replace the sentinel with real specification or research entries containing valid file and reason fields, using the packages context listing if needed; remove the template line from every manifest..trellis/tasks/archive/2026-08/07-27-guard-break-loop-artifact-paths/task.json-15-16 (1)
15-16: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winArchived task records omit branch provenance.
Both records use
branch: nullwithbase_branch: "main". The archive validator treats this as missing branch metadata in a repository with a Git remote.
.trellis/tasks/archive/2026-08/07-27-guard-break-loop-artifact-paths/task.json#L15-L16: record the feature branch or document a local-only task..trellis/tasks/archive/2026-08/08-08-create-empty-metadata-rejection/task.json#L15-L16: record the feature branch or document a local-only task.🤖 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 @.trellis/tasks/archive/2026-08/07-27-guard-break-loop-artifact-paths/task.json around lines 15 - 16, Update the archived task metadata so both records preserve branch provenance: in .trellis/tasks/archive/2026-08/07-27-guard-break-loop-artifact-paths/task.json lines 15-16 and .trellis/tasks/archive/2026-08/08-create-empty-metadata-rejection/task.json lines 15-16, replace branch: null with the originating feature branch, or explicitly document that each task was local-only in the format accepted by the archive validator; retain base_branch as main..trellis/tasks/archive/2026-08/08-08-task-rename/prd.md-19-29 (1)
19-29: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the dangling-reference acceptance test with requirement 2.
Lines 19-21 permit references outside the task directory to remain unchanged and only be reported. Lines 27-29 require zero dangling references. Define whether the scripted scan excludes report-only references or whether the command must rewrite them. Otherwise a correct implementation can fail its own acceptance test.
🤖 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 @.trellis/tasks/archive/2026-08/08-08-task-rename/prd.md around lines 19 - 29, Clarify the acceptance criteria for dangling references to match requirement 2: specify that the scripted scan excludes report-only references outside the task directory, or change the requirement so those references are rewritten. Ensure the documented behavior consistently defines which references must be absent after a successful rename..trellis/tasks/archive/2026-08/08-08-task-rename/check.jsonl-1-1 (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCompleted task manifests still contain the
_examplesentinel. Replace each sentinel with real specification or research entries, or remove the manifest when no context is required.
.trellis/tasks/archive/2026-08/08-08-task-rename/check.jsonl#L1-L1: Add valid context entries for thecheckphase..trellis/tasks/archive/2026-08/08-08-task-rename/implement.jsonl#L1-L1: Add valid context entries for theimplementphase.🤖 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 @.trellis/tasks/archive/2026-08/08-08-task-rename/check.jsonl at line 1, Replace the _example sentinel in .trellis/tasks/archive/2026-08/08-08-task-rename/check.jsonl at lines 1-1 with valid specification or research context entries for the check phase. Replace the same sentinel in .trellis/tasks/archive/2026-08/08-08-task-rename/implement.jsonl at lines 1-1 with valid entries for the implement phase, or remove either manifest if no context is required; include only spec/research files and no code paths..trellis/tasks/archive/2026-08/08-08-task-rename/prd.md-16-24 (1)
16-24: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine crash recovery for the atomic rename.
Lines 16-24 require “one operation”, but the command changes a directory and multiple JSON, JSONL, and journal references. Filesystem rename and multiple file writes cannot commit as one atomic filesystem transaction. Specify staging, rollback, or a recovery marker, and add fault-injection tests after each write boundary.
🤖 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 @.trellis/tasks/archive/2026-08/08-08-task-rename/prd.md around lines 16 - 24, Define crash-recovery semantics for the atomic rename described in task.py rename, including staging, rollback, or a durable recovery marker covering directory, JSON, JSONL, and journal updates. Specify how interrupted operations are detected and resumed or reverted, then add fault-injection tests after each filesystem or file-write boundary to verify recovery and preserve --dry-run behavior..trellis/tasks/archive/2026-08/07-28-harden-add-session-retry-convergence/design.md-12-20 (1)
12-20: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the retry fingerprint stable across date boundaries.
Lines 12-20 include the wall-clock date in the retry key. The supplied
.trellis/scripts/add_session.pyimplementation computestodayimmediately beforecompute_record_fingerprint. If the journal write succeeds before midnight and index or commit fails, a retry after midnight receives a different marker and appends a second session. Exclude the wall-clock date from retry identity, or reuse the pending record’s persisted date, and add a boundary test.🤖 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 @.trellis/tasks/archive/2026-08/07-28-harden-add-session-retry-convergence/design.md around lines 12 - 20, The retry fingerprint computation in add_session.py must remain stable across midnight: exclude the wall-clock date from compute_record_fingerprint, or reuse the pending record’s persisted date when retrying. Add a boundary test covering a journal write before midnight followed by an index or commit failure and retry after midnight, ensuring no duplicate session is appended..trellis/tasks/archive/2026-08/07-27-validate-task-branch-metadata-before-archive/prd.md-70-73 (1)
70-73: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winArchived task provenance includes host-specific and session-identifying data. Remove or redact it before committing these records.
.trellis/tasks/archive/2026-08/07-27-validate-task-branch-metadata-before-archive/prd.md#L70-L73: Replace the Codex thread ID and absolute/Users/sven/repos/platypeeps/sd-ai-command-packpath with redacted or repository-relative provenance..trellis/tasks/archive/2026-08/07-27-validate-task-branch-metadata-before-archive/task.json#L31-L35: Remove or redactmeta.source_repoandmeta.source_thread_id, or store raw values only in ignored workspace state.🤖 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 @.trellis/tasks/archive/2026-08/07-27-validate-task-branch-metadata-before-archive/prd.md around lines 70 - 73, Remove or redact the host-specific path and Codex thread ID from .trellis/tasks/archive/2026-08/07-27-validate-task-branch-metadata-before-archive/prd.md lines 70-73, retaining only repository-relative or non-identifying provenance. In .trellis/tasks/archive/2026-08/07-27-validate-task-branch-metadata-before-archive/task.json lines 31-35, remove or redact meta.source_repo and meta.source_thread_id, or move their raw values to ignored workspace state..trellis/tasks/archive/2026-08/07-28-harden-add-session-retry-convergence/design.md-53-59 (1)
53-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize session-number allocation before writing.
Both runtime scripts call
get_latest_journal_info,resolve_next_session, andwrite_text_atomicwithout a lock or compare-and-retry guard spanning the full sequence. Atomic replacement alone can let two writers choose the same session number or overwrite journal/index updates. Add guard coverage to both.trellis/scripts/add_session.pyand the cli template copy.🤖 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 @.trellis/tasks/archive/2026-08/07-28-harden-add-session-retry-convergence/design.md around lines 53 - 59, Update both add-session implementations—.trellis/scripts/add_session.py and its CLI template copy—to serialize the full get_latest_journal_info → resolve_next_session → write_text_atomic sequence with a lock or compare-and-retry guard. Ensure concurrent writers cannot allocate the same session number or overwrite journal/index updates, while keeping any lock released before Git execution..trellis/tasks/archive/2026-08/07-28-harden-add-session-retry-convergence/prd.md-71-88 (1)
71-88: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRequire collision detection in the numbering design.
A union of local and default-branch journals is only a snapshot. Two branches can read the same snapshot and choose the same next number. Make collision-detecting retry or an atomic allocator mandatory. Add an acceptance test for concurrent branch recordings.
Proposed specification update
- e.g., derive from union of local + default-branch journals + use a collision-detecting retry or atomic allocator; a read-only union is insufficient + [ ] Concurrent branch recordings converge to unique session numbers after merge/retryAlso applies to: 99-109
🤖 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 @.trellis/tasks/archive/2026-08/07-28-harden-add-session-retry-convergence/prd.md around lines 71 - 88, Update the acceptance criteria for session numbering to require collision detection through retry or an atomic allocator when concurrent branches record from the same journal snapshot. Add an acceptance test covering concurrent branch recordings and verify that they receive distinct session numbers without corrupting or overwriting records..trellis/tasks/archive/2026-08/07-28-restore-install-safe-opencode-mem-reader/prd.md-45-47 (1)
45-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the missing-storage warning contract.
R7 requires structured
missingwarnings, while the design says missing storage produces no warning. Choose one policy, then update the PRD, design, OpenCode adapter, CLI rendering, and tests so missingopencode.dbbehaves consistently.🤖 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 @.trellis/tasks/archive/2026-08/07-28-restore-install-safe-opencode-mem-reader/prd.md around lines 45 - 47, Resolve the missing-storage warning contract by choosing one policy and applying it consistently: update the PRD R7 requirements and design guidance, then align the OpenCode adapter, CLI rendering, and tests so missing opencode.db produces the same documented behavior everywhere. Affected sites are prd.md lines 45-47 and design.md lines 52-54; update both directly, along with the corresponding adapter, renderer, and test implementations.
🧹 Nitpick comments (8)
packages/core/test/mem/adapters.test.ts (1)
1396-1414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the python error output when a fixture build fails.
runPythonsetsstdio: "ignore".execFileSyncstill throws on a non-zero exit, but the thrown error carries no python traceback. A broken fixture script then fails with an opaque message. Capturestderrand attach it to the thrown error.♻️ Proposed change to keep the python traceback
try { - execFileSync(pyCmd, [pyFile], { - stdio: "ignore", - maxBuffer: 64 * 1024 * 1024, - }); + try { + execFileSync(pyCmd, [pyFile], { + stdio: ["ignore", "ignore", "pipe"], + maxBuffer: 64 * 1024 * 1024, + }); + } catch (error) { + const stderr = (error as { stderr?: Buffer }).stderr?.toString() ?? ""; + throw new Error(`python fixture failed:\n${stderr}`, { cause: error }); + } } finally { nodeFs.rmSync(pyDir, { recursive: true, force: true }); }🤖 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 `@packages/core/test/mem/adapters.test.ts` around lines 1396 - 1414, Update runPython to capture Python stderr instead of ignoring it, while preserving the existing cleanup in the finally block. When execFileSync fails, include the captured traceback in the thrown error so fixture build failures expose the Python diagnostic output.packages/core/test/mem/api.test.ts (1)
218-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared python detector and OpenCode fixture path into a test helper.
findPythonForSqliteandOPENCODE_DBare duplicated here and inpackages/core/test/mem/adapters.test.ts(Lines 1377-1422). The two detectors also differ in return type: this one returnsstring | null, the other returnsstring[] | null. Move one implementation into a shared module underpackages/core/test/mem/and import it in both suites. That keeps the skip condition and the fixture path identical across the SQLite-backed tests.🤖 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 `@packages/core/test/mem/api.test.ts` around lines 218 - 242, Extract findPythonForSqlite and the OPENCODE_DB fixture path into a shared helper module under packages/core/test/mem/, preserving one consistent detector return type and implementation. Update both api.test.ts and adapters.test.ts to import and reuse the helper, and remove their duplicated local definitions so SQLite skip conditions and fixture paths remain identical.packages/cli/src/templates/common/skills/brainstorm.md (1)
165-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTerminology drift with Line 191.
Line 165 now describes an uncurated manifest as "an empty manifest, or one holding only a legacy
_exampleplaceholder row". Line 191 in the Quality Bar still says "seed-only manifests are not ready". Use one term for one concept so an agent applies the same gate in both sections.🤖 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 `@packages/cli/src/templates/common/skills/brainstorm.md` at line 165, Align the terminology between the manifest gate at line 165 and the Quality Bar near line 191: replace “seed-only manifests” in the Quality Bar with the established description covering empty manifests and manifests containing only the legacy `_example` placeholder, while preserving the requirement for at least one real spec/research entry..trellis/scripts/common/task_store.py (1)
927-936: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the atomic writer for the JSONL rewrites.
Every other write in
_apply_renamegoes throughwrite_json, which writes to a temp file and renames. The JSONL rewrite usesPath.write_text, which truncates the target first. An interruption there leaves a truncated manifest, which the documented "run the same rename again" recovery cannot repair because the old paths are gone.io.write_text_atomicalready provides the same guarantee for text files.♻️ Proposed refactor
for jsonl_path, _linenos, text in plan.jsonl: - try: - jsonl_path.write_text(text, encoding="utf-8") - except OSError as exc: - print( - colored(f"Error: Failed to write {jsonl_path}: {exc}", Colors.RED), - file=sys.stderr, - ) - _rename_interrupted(plan) - return 1 + if not write_text_atomic(jsonl_path, text): + print( + colored(f"Error: Failed to write {jsonl_path}", Colors.RED), + file=sys.stderr, + ) + _rename_interrupted(plan) + return 1Add
write_text_atomicto the existingfrom .io import ...line.🤖 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 @.trellis/scripts/common/task_store.py around lines 927 - 936, Replace the direct Path.write_text call in _apply_rename with the existing io.write_text_atomic helper, importing write_text_atomic alongside the other .io utilities. Preserve the current encoding, error handling, interrupted-plan rename, and return behavior.packages/cli/src/templates/trellis/scripts/add_session.py (2)
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive
STATE_COMMITTEDandCOMMIT_DONEdistinct values.Both constants equal
"committed". They belong to two different families.stateis compared againstSTATE_*andoutcomeis compared againstCOMMIT_*. The current code never crosses the two families, so there is no defect today. A future cross-comparison would compare equal by accident and pass silently. Distinct values make that mistake fail loudly.♻️ Proposed change
# Auto-commit outcomes. -COMMIT_DONE = "committed" +COMMIT_DONE = "commit-done"🤖 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 `@packages/cli/src/templates/trellis/scripts/add_session.py` around lines 98 - 108, Assign distinct string values to STATE_COMMITTED and COMMIT_DONE while preserving their respective state and outcome semantics; update only these constants so cross-family comparisons cannot match accidentally.
572-597: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish a failed convergence probe from "no sessions found".
run_gitreturns(1, "", <exception text>)when the command raises, including on aGIT_PROBE_TIMEOUTexpiry. Line 588 mapsrc > 1 or not outto0, and rc 1 with empty output also yields0. A timed-out probe is therefore indistinguishable from a clean "no match".The failure is silent and biases in the unsafe direction:
resolve_next_sessionthen trusts the working tree alone and can reissue a session number that another branch already used. Print a stderr warning when the probe does not complete, so the operator can see that convergence was skipped.♻️ Proposed change
- rc, out, _ = run_git( + rc, out, err = run_git( [ "grep", @@ cwd=repo_root, timeout=GIT_PROBE_TIMEOUT, ) - if rc > 1 or not out: + if rc > 1: + print( + "[WARN] Could not scan other refs for recorded sessions " + f"({err.strip() or 'git grep failed'}); session numbering falls " + "back to the working tree and may collide with a parallel branch.", + file=sys.stderr, + ) + return 0 + if not out: return 0🤖 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 `@packages/cli/src/templates/trellis/scripts/add_session.py` around lines 572 - 597, Update the session-number probe around run_git in resolve_next_session’s helper so any nonzero return code is treated as an incomplete probe, including rc == 1 with empty output. Emit a stderr warning containing the returned error details before returning 0, while preserving the existing no-match behavior for a successful probe with no matches..trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/prd.md (1)
17-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the exact developer identity precedence.
Lines 17-21 name the environment, local checkout, and main-worktree sources but do not define their order. The supplied resolver uses
TRELLIS_DEVELOPER, then local.trellis/.developer, then the main-worktree.trellis/.developer;--assigneeoverrides before resolution. Record this order and test each source.🤖 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 @.trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/prd.md around lines 17 - 21, Update the developer identity resolution section to document the exact precedence: apply --assignee first, then check TRELLIS_DEVELOPER, the local .trellis/.developer, and finally the main-worktree .trellis/.developer. Add or update tests covering each source and confirming the documented order..trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/task.json (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompleted task records lose implementation traceability because
relatedFilesis empty.
.trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/task.json#L23-L23: List.trellis/scripts/common/paths.pyand its packaged template counterpart..trellis/tasks/archive/2026-08/08-08-task-rename/task.json#L23-L23: List.trellis/scripts/task.py, the affected common modules, and their packaged template counterparts.🤖 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 @.trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/task.json at line 23, Populate relatedFiles in .trellis/tasks/archive/2026-08/08-08-developer-worktree-provisioning/task.json#L23-L23 with .trellis/scripts/common/paths.py and its packaged template counterpart. Also update .trellis/tasks/archive/2026-08/08-08-task-rename/task.json#L23-L23 to list .trellis/scripts/task.py, the affected common modules, and their packaged template counterparts.
- add_session: journal session-heading regex now MULTILINE so numbering sees all prior sessions, not just a heading at offset 0 - add_session: local branch heads exempt from convergence ref cap; warn when the remote tail is truncated - paths: reject traversal-shaped developer names (.., separators, NUL) from env and identity file before joining under workspace/ - task rename: repoint session runtime files that referenced the old task path so `task.py current` survives a rename - trellis_config: reject mapping-shaped list items instead of storing the first key as a corrupt string entry - mem/opencode: clamp out-of-range timestamps instead of throwing RangeError from toISOString - mem/paths: expand ~ and resolve relative OPENCODE_DB against the data directory Both script trees kept in byte parity; spec ref-cap paragraph updated; regression + integration coverage added for each fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Contributor
Author
|
Review convergence for the CodeRabbit pass — all findings verified against the code before acting. Fixed in b21a667 (each reproduced first, regression/integration coverage added):
Not changing, with reasons:
Both script trees ( |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
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 @.trellis/scripts/common/active_task.py:
- Around line 742-752: Update the session-repoint helper in
.trellis/scripts/common/active_task.py (lines 742-752) and the packaged template
at packages/cli/src/templates/trellis/scripts/common/active_task.py (lines
742-752) to track unreadable session files and failed _write_json calls,
returning both successful repoints and failed session paths. Update the rename
handling in .trellis/scripts/common/task_store.py (lines 954-959) to consume
that failure status and return a nonzero recovery-required result when any
session repoint is incomplete.
In @.trellis/scripts/common/paths.py:
- Around line 93-96: Update _safe_developer_name in both
.trellis/scripts/common/paths.py (lines 93-96) and
packages/cli/src/templates/trellis/scripts/common/paths.py (lines 93-96) to
reject names containing ":" alongside the existing invalid-name checks, before
returning the name. This must prevent Windows drive-qualified names such as "C:"
from escaping the workspace.
In `@packages/cli/test/scripts/add-session.integration.test.ts`:
- Around line 797-800: Update the stale-index setup in the add-session
integration test after runAddSession to read the index once, assert that the
“**Total Sessions**: 1” marker is present, then write the replaced content via
indexPath(tmp). Ensure the test cannot silently continue when the expected
marker is absent.
🪄 Autofix
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 Plus
Run ID: 51a4666a-dd69-4d8c-b8d4-5f45e146b224
📒 Files selected for processing (20)
.trellis/scripts/add_session.py.trellis/scripts/common/active_task.py.trellis/scripts/common/paths.py.trellis/scripts/common/task_store.py.trellis/scripts/common/trellis_config.py.trellis/spec/cli/backend/script-conventions.md.trellis/tasks/08-09-hook-timeout-process-tree/check.jsonl.trellis/tasks/08-09-hook-timeout-process-tree/implement.jsonl.trellis/tasks/08-09-hook-timeout-process-tree/prd.md.trellis/tasks/08-09-hook-timeout-process-tree/task.jsonpackages/cli/src/templates/trellis/scripts/add_session.pypackages/cli/src/templates/trellis/scripts/common/active_task.pypackages/cli/src/templates/trellis/scripts/common/paths.pypackages/cli/src/templates/trellis/scripts/common/task_store.pypackages/cli/src/templates/trellis/scripts/common/trellis_config.pypackages/cli/test/regression.test.tspackages/cli/test/scripts/add-session.integration.test.tspackages/core/src/mem/adapters/opencode.tspackages/core/src/mem/internal/paths.tspackages/core/test/mem/adapters.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/mem/internal/paths.ts
- packages/cli/src/templates/trellis/scripts/common/trellis_config.py
- .trellis/scripts/common/trellis_config.py
- packages/cli/src/templates/trellis/scripts/add_session.py
- .trellis/spec/cli/backend/script-conventions.md
- packages/cli/src/templates/trellis/scripts/common/task_store.py
- .trellis/scripts/add_session.py
…tion CodeRabbit incremental review on PR 534: - paths: a name like "C:" carries no separator but joins as a drive-relative path outside workspace/ on Windows. _safe_developer_name now rejects names containing ":". - add-session test: assert the "**Total Sessions**: 1" marker exists before the replace() that downgrades it, so the stale-index condition the test claims to cover is actually created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Contributor
Author
CI failure: action needed in
|
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 @.trellis/workspace/sven/journal-1.md:
- Around line 115-121: Update the Git Commits section in the journal to record
the commit provenance for the nine reported fixes: if commit b21a6675 contains
them, list that hash and describe the session as verification; otherwise replace
the no-commits note with every commit created during this session. Keep the
existing findings summary unchanged.
🪄 Autofix
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 Plus
Run ID: cecce625-d00d-40dd-ad74-c557745206c8
📒 Files selected for processing (2)
.trellis/workspace/sven/index.md.trellis/workspace/sven/journal-1.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .trellis/workspace/sven/index.md
subprocess.run's timeout kills only the shell; grandchildren survived, and one holding the captured pipes blocked the post-kill collect indefinitely — the hang the timeout exists to prevent. run_task_hooks now spawns hooks via Popen with start_new_session on POSIX and, on timeout, kills the process group (SIGKILL via killpg; taskkill /F /T on Windows, proc.kill() fallback on both). Post-kill output collection is bounded by HOOK_KILL_GRACE_SECONDS; if an unkillable orphan still holds a pipe, the streams are abandoned instead of waited on. Happy-path and non-zero-exit behavior unchanged. A hook that calls setsid itself escapes the group — documented as a known limitation in script-conventions.md. Raised by CodeRabbit on PR mindfold-ai#534; implemented as follow-up task 08-09-hook-timeout-process-tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/test/regression.test.ts (1)
11719-11725: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelax the assertions that pin literal values.
expect(commonTaskUtils).toContain("HOOK_KILL_GRACE_SECONDS = 5")fails when the grace period changes, even though the process-tree behavior under test is unchanged. Match the declaration shape instead of the value. Thetaskkillassertion has the same fragility for quote style and argument spacing, but it is the only available check for the Windows branch, so keeping it exact is reasonable.♻️ Proposed change
- expect(commonTaskUtils).toContain("HOOK_KILL_GRACE_SECONDS = 5"); + expect(commonTaskUtils).toMatch(/^HOOK_KILL_GRACE_SECONDS = \d+$/m); expect(commonTaskUtils).toContain("timeout=HOOK_KILL_GRACE_SECONDS");🤖 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 `@packages/cli/test/regression.test.ts` around lines 11719 - 11725, Relax the grace-period assertion in the regression test by matching the HOOK_KILL_GRACE_SECONDS declaration shape without pinning its numeric value, while retaining the timeout=HOOK_KILL_GRACE_SECONDS check. Leave the exact taskkill assertion unchanged because it remains the Windows-branch coverage check.
🤖 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.
Nitpick comments:
In `@packages/cli/test/regression.test.ts`:
- Around line 11719-11725: Relax the grace-period assertion in the regression
test by matching the HOOK_KILL_GRACE_SECONDS declaration shape without pinning
its numeric value, while retaining the timeout=HOOK_KILL_GRACE_SECONDS check.
Leave the exact taskkill assertion unchanged because it remains the
Windows-branch coverage check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f0e1b00-5971-4fbe-a80a-b50948f19e86
📒 Files selected for processing (8)
.trellis/scripts/common/task_utils.py.trellis/spec/cli/backend/script-conventions.md.trellis/tasks/archive/2026-08/08-09-hook-timeout-process-tree/check.jsonl.trellis/tasks/archive/2026-08/08-09-hook-timeout-process-tree/implement.jsonl.trellis/tasks/archive/2026-08/08-09-hook-timeout-process-tree/prd.md.trellis/tasks/archive/2026-08/08-09-hook-timeout-process-tree/task.jsonpackages/cli/src/templates/trellis/scripts/common/task_utils.pypackages/cli/test/regression.test.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
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
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.
Summary
Batch of hardening and feature work on the Trellis script runtime and mem subsystem, developed task-by-task with the Trellis workflow (each feature has an archived task with PRD/design artifacts under
.trellis/tasks/archive/2026-08/).Script runtime hardening (from the runtime-hardening audit)
resolve_task_dir: rejects paths outside.trellis/tasks/with symlinks resolved first;find_task_by_namerejects separators and ambiguous suffixes (5a1d59e)io.py(1cf22b5)task.py createbefore any filesystem write (e1a1798)index.lock(a95e748)Task lifecycle features
task.py renamewith atomic identity + back-reference rewrite and--dry-run(f8d5de5)task.py start; branch metadata validated before archive (00ae5af)--git-common-dirinheritance (0740d1d)task.py validaterejects legacy_exampleplaceholder rows and non-object JSONL rows, aligning local validation with downstream PR preflight (0b6577d)add_session.pyrewritten as a resumable state machine: commit OIDs resolved to real subjects before any mutation (no more(see git log)), fingerprint markers give retries an exact re-entry point (journal → index → scoped commit), auto-commit failure returns a checkpoint instead of false success, session numbering converges across concurrent branches, atomic journal/index writes (76c53c5)Mem
better-sqlite3regression stays out). Name-matched schema validation, WAL-consistent snapshots, structured warnings,parent_idchild merging (adb7acf)Also
trellis-implement/trellis-researchagents pinned to Opus in dogfood and packaged templates (3956711)Testing
python3)diff -rq .trellis/scripts packages/cli/src/templates/trellis/scripts)🤖 Generated with Claude Code
https://claude.ai/code/session_017Qa6GmqwXdKGd19d2b8oqP
Summary by CodeRabbit