Skip to content

feat(profile): resolve remote skills for worktree agents - #663

Merged
drewstone merged 1 commit into
mainfrom
feat/analyst-improvement-proof
Jul 30, 2026
Merged

feat(profile): resolve remote skills for worktree agents#663
drewstone merged 1 commit into
mainfrom
feat/analyst-improvement-proof

Conversation

@drewstone

@drewstone drewstone commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What changed

  • resolve GitHub-backed AgentProfile resources before worktree worker launch
  • mount exact skill bytes without mutating the source profile or candidate patch
  • propagate cancellation through resource fetches and clean failed worktrees
  • document immutable remote skill references

Proof

  • pnpm test: 1,881 passed, 6 skipped
  • pnpm typecheck: passed
  • pnpm lint: 502 files passed
  • pnpm verify:package: passed, including packed exports and edge execution
  • git merge-tree --write-tree origin/main HEAD: clean

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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.

value-audit · 20260730T085053Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — d95251ea

Review health 100/100 · Reviewer score 77/100 · Confidence 80/100 · 6 findings (6 low)

glm: Correctness 77 · Security 77 · Testing 77 · Architecture 77

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.

🟡 LOW AbortSignal.any() is experimental on the declared Node engine floor (22.13.0) — src/mcp/worktree-harness.ts

package.json declares "engines": { "node": ">=22.13.0" }. AbortSignal.any() was only made non-experimental in Node 22.14.0 (and 20.18.3 / 23.2.0). On the minimum-supported 22.13.0 it still functions but prints an ExperimentalWarning to stderr on first use. Impact is cosmetic noise on the oldest supported runtime, not a correctness defect. Fix options: bump engine floor to >=22.14.0, or wrap with a tiny combineSignals helper that falls back to a manual AbortController listener when AbortSignal.any is absent/unstable. Verified locally on Node 24 it resolves cleanly.

🟡 LOW New resourceResolution option is not plumbed by either caller; GitHub refs without their own repository will throw at resolution — src/mcp/worktree-harness.ts

The new resourceResolution field exposes { repository?, fetch?, timeoutMs? } (ResolveAgentProfileResourcesOptions from agent-profile-materialize@0.9.2). Neither caller wires it: worktree-cli-executor.ts:147 and in-process-executor.ts:147 construct RunWorktreeHarnessOptions without resourceResolution, so opts.resourceResolution is always undefined in production. Consequence: resolveAgentProfileResources runs with no default repository, and the upstream throws 'GitHub profile resource "" requires repository' (index.js:330) for any resource ref that omits its own repository field. Profiles relying on an inherited default repo will fail at this new call site where they previously did not reach a fetch. This is an integration gap in the callers (outside this shot's files), but the option

🟡 LOW Resource resolution runs after worktree creation, so a fetch failure wastes a git create+remove cycle — src/mcp/worktree-harness.ts

createWorktree() at :216 runs before resolveAgentProfileResources() at :233. When resolution throws (network error, MAX_PROFILE_RESOURCE_BYTES exceeded, missing repository default, invalid ref), the catch at :334 correctly calls cleanup() to remove the just-created worktree — so there is no leak. But every failed resolution now performs a redundant branch creation + worktree checkout + remove. Moving the resolveAgentProfileResources call (and assertSafeProfileResourcePaths, which only reads profile.resources.files paths) above createWorktree would avoid the wasted git operations on the failure path. The worktree path does not depend on resolved resources. Not a correctness issue; ordering is a design choice.

🟡 LOW resolveResourceInstructions throw for non-inline instructions is now defensively unreachable — src/mcp/worktree-harness.ts

After resolveAgentProfileResources runs (upstream index.js:401-403 converts object instructions to {kind:'inline', content:string}), resources.instructions is either undefined, a string, or {kind:'inline', content:string}. The throw at :374 ('remote refs require pre-resolution') can no longer fire for the instructions axis through runWorktreeHarness. Keeping it as a defensive guard for direct callers is acceptable, but the error message is now stale — it implies the caller must pre-resolve, when in fact runWorktreeHarness now does the pre-resolution itself. Cosmetic; no behavior impact.

🟡 LOW Production 'no resourceResolution provided' path no longer unit-tested — tests/mcp/worktree-harness.test.ts

The replaced test 'rejects unresolved resource instructions' asserted failure when NO resourceResolution.fetch was supplied (forcing the default globalThis.fetch path). The new test always injects a mocked fetch returning 404. The default-fetch production code path (resourceResolutionOptions(undefined, signal) → wrapped globalThis.fetch) is therefore not directly exercised in this file. Acceptable trade-off: the new test is deterministic where the old one would have made a real network call, and the abort-signal forwarding logic IS covered by the cancellation test. Flagging only so the gap is conscious.

🟡 LOW core.hooksPath '/dev/null' is POSIX-only — tests/mcp/worktree-harness.test.ts

git(repoRoot, ['config', 'core.hooksPath', '/dev/null']) assumes a POSIX /dev/null. On Windows the path is invalid and would either be ignored by git or fail the config write; the existing test at line 99 (if (process.platform === 'win32') return) shows Windows is at least considered in this file. Non-blocking because .github/workflows/ci.yml runs only on ubuntu-latest, but if local Windows contributors run this suite with a global core.hooksPath set, behavior is undefined. Cheap fix: gate with process.platform !== 'win32' or use a known-empty directory under tmpdir().


tangletools · 2026-07-30T08:58:43Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

@drewstone
drewstone merged commit 2326075 into main Jul 30, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants