feat(profile): resolve remote skills for worktree agents - #663
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — d95251ea
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-30T08:48:01Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 152.7s (2 bridge agents) |
| Total | 152.7s |
💰 Value — sound
Wires the materialize library's existing remote-resource resolver into the single worktree-harness core so GitHub-backed skills/files/instructions mount before worker launch — exactly the pre-resolution step the library's fail-closed contract already mandated, with correct cancellation propagation a
- What it does: Before: runWorktreeHarness passed opts.profile straight to materializeProfile, so any profile carrying a GitHub resource ref (skills/files/agents/commands/instructions) hit the library's fail-closed 'github refs need async pre-resolution' branch and threw. After (src/mcp/worktree-harness.ts:233-237): it first calls resolveAgentProfileResources(opts.profile, ...) — the library function that fetches
- Goals it achieves: (1) Let worktree worker agents actually receive remotely-defined skills/files instead of failing — e.g. a pinned Traces skill ref mounts at .codex/skills//SKILL.md. (2) Keep the source profile and the returned candidate patch untouched: resolved bytes are a local copy, worker edits to mounted inputs never appear in the diff. (3) Make remote fetches honour caller cancellation and clean up the
- Assessment: Good change, in the grain of the codebase. The materialize library deliberately splits async resolution from pure materialization and its own unsupported messages prescribe exactly 'github refs need async pre-resolution' (lib dist/index.js:501,510,523,569,597); this PR makes the harness perform that prescribed step. Placing it inside runWorktreeHarness — the documented 'ONE worktree-harness execut
- Better / existing approach: none — this is the right approach. Checked: (a) the materialize library cannot do this itself because materializeProfile is explicitly documented 'Pure — no IO' (dist/index.d.ts:185-188) and resolution is the separate async sibling it exports precisely for this; (b) resolving at the executor layer instead of the core would force both in-process-executor.ts:147-161 and worktree-cli-executor.ts:147-
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound
Moves GitHub-backed skill/resource resolution into the single worktree execution core so it runs for every worker launch by default, removing a pre-resolution burden the old code and callers explicitly carried.
- Integration: Fully reachable and active by default: runWorktreeHarness (the one core, called by both in-process-executor.ts:147 and worktree-cli-executor.ts:147) calls resolveAgentProfileResources unconditionally at worktree-harness.ts:233. When profile.resources is absent the resolver is a no-op passthrough (agent-profile-materialize index.js:302-303), so existing resourceless profiles are unaffected; profile
- Fit with existing patterns: Fits the codebase grain. GitHub resource refs are first-class (candidate-execution/profile.ts:408 constructs {kind:'github',repository,path,ref}). The old core refused remote refs ('remote refs require pre-resolution' at worktree-harness.ts:375; personify/corpus.ts:325 tells callers to pre-resolve). This PR moves resolution into the same file that already owns profile materialization and assertSaf
- Real-world viability: Holds up on the non-happy paths. Cancellation is correctly threaded: resourceResolutionOptions checks throwIfAborted before each fetch and composes the caller signal with the per-request signal via AbortSignal.any (safe under engines node>=22.13.0). Both failure modes are tested — 404 throws before worker launch with worktree cleanup (worktree-harness.test.ts:559), and mid-fetch abort rejects with
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
No concerns — sound change, no better or existing approach found. ✅
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 6 non-blocking findings — d95251ea
Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-30T08:58:43Z · immutable trace
What changed
Proof
pnpm test: 1,881 passed, 6 skippedpnpm typecheck: passedpnpm lint: 502 files passedpnpm verify:package: passed, including packed exports and edge executiongit merge-tree --write-tree origin/main HEAD: clean