feat(daemon): install skills in cluster sandboxes - #1545
Conversation
![agentconnect-md-test[bot]](https://avatars.githubusercontent.com/in/4157507?s=60&v=4){kind=small}
There was a problem hiding this comment.
Fast pass, two blocking issues found in the new cluster-skill/duty-fencing code:
-
DutyCoordinator.revokeProjectedFencesdrops fences for all but the first group in a batch revoke (duty-coordinator.ts:534, mirrored at:559). The caller kicks offrevokeProjectedFences(groupIds)without awaiting it, then immediately calls the synchronousthis.duties.applyRevoke(...), which deletes those groupIds from the in-memory map.revokeProjectedFences's loop only readsthis.duties.get(groupId)for the first entry before it hits its firstawait(insidehost.revokeDutyWriteFence); once resumed, subsequent iterations readthis.duties.get(groupId)for a now-deleted entry, getundefined, andcontinue— sorevokeDutyWriteFence(which deletes the durableduty_write_fencerow inlocal-store.ts) never fires for groups 2..N in any multi-group revocation or self-fence batch. That leaves a stale write-fence row pointing at a daemon/term that no longer holds the duty. Fix: snapshot eachheld.termbefore callingapplyRevoke, or awaitrevokeProjectedFencesbefore mutatingthis.duties. -
Likely typecheck failure in
skill-install-ledger.ts:171:return ledger?.publicationOperationId === operationId && publicationMacValid(ledger, publicationKey)—ledgerisSkillInstallLedger | null. TS only narrows the base of an optional chain when the chain itself is the truthy-checked condition (as correctly done a few lines later at:400,ledger?.publicationOperationId && ...); an===comparison doesn't narrow it, so passingledgerstraight intopublicationMacValid(ledger: SkillInstallLedger, ...)should be a type error. Needs an explicitledger !== null &&guard (or non-null assertion) before thepublicationMacValidcall.
Rest of the new protocol/staging code (skill-protocol.ts, skill-handler.ts) looks carefully defended — path traversal, symlink, size, and offset checks all look sound.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
| // that admission, or the member starts serving a group the CP has already taken away. | ||
| const pending = this.withdrawDutyGroups(revocations.map((revocation) => revocation.groupId)) | ||
| if (pending.length > 0) this.log.info(`duty: revoked ${pending.length} group(s) mid-admission`) | ||
| const fenceRevocation = this.revokeProjectedFences(revocations.map((one) => one.groupId)) |
There was a problem hiding this comment.
revokeProjectedFences(revocations.map(...)) isn't awaited before this.duties.applyRevoke(revocations) runs synchronously right after. Since revokeProjectedFences's for-loop only reads this.duties.get(groupId) (for held.term) up to its first await, every group after the first sees its entry already deleted by applyRevoke and silently skips the revokeDutyWriteFence call via the if (!held) continue guard. Same pattern at fenceDuties (line 559).
There was a problem hiding this comment.
Fixed in f037187. The held {groupId, term} tuples are now snapshotted synchronously before applyRevoke mutates the registry, and both revoke paths consume that snapshot. Added a multi-group self-fence regression test asserting both durable fence revocations; daemon typecheck and the 15-test duty-fence suite pass.
There was a problem hiding this comment.
Confirmed — that matches what I found when I reviewed f037187c directly: projectedFences() now snapshots {groupId, term} synchronously before applyRevoke mutates the registry, so the fence-revocation loop no longer races the map deletion, and the new multi-group self-fence test in daemon-duty-fence.test.ts pins exactly that behavior for both groups. Good fix — thanks for closing the loop on this one.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. Snapshotting every {groupId, term} before applyRevoke removes the map-mutation race, and the multi-group regression test covers the failure mode. Good fix—thanks.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| ): Promise<boolean> { | ||
| const location = await skillLedgerLocation(cwd, stateDir) | ||
| const ledger = await readSkillLedger(location) | ||
| return ledger?.publicationOperationId === operationId && publicationMacValid(ledger, publicationKey) |
There was a problem hiding this comment.
ledger is SkillInstallLedger | null here; ledger?.publicationOperationId === operationId doesn't narrow it (unlike the truthy-chain check at line 400), so publicationMacValid(ledger, publicationKey) looks like a type error — ledger needs an explicit non-null check first.
There was a problem hiding this comment.
Clarified in f037187 with an explicit ledger !== null guard. The existing compiler accepted the prior expression, but the explicit guard makes the narrowing and runtime precondition unambiguous. Daemon typecheck passes.
There was a problem hiding this comment.
You're right, and I owe you a correction. Let me re-examine this properly instead of restating my earlier claim.
TypeScript's control-flow analysis has specific support for narrowing the base of an optional chain when the chain is compared (via ===/!==) against a value that can't itself be undefined — this is the "discriminant narrowing through optional-chain containment" behavior, and it applies here: since operationId is typed string (never undefined), ledger?.publicationOperationId === operationId being true is only possible when ledger is non-null, and the compiler does narrow on that basis. That's different from a plain equality check on an unrelated variable, which I incorrectly generalized from. So the original expression at line 171 was in fact sound, and pnpm --filter @agentconnect.md/daemon typecheck passing on it isn't surprising — I was wrong to flag it as a likely type error.
I should have verified this against the actual compiler rather than reasoning it out under sandbox constraints (no deps/network here) and presenting it with more confidence than the evidence supported. Sorry for the noise — the explicit ledger !== null guard you added is still a reasonable clarity improvement, just not one that was fixing a real bug. This doesn't change anything about the duty-fencing finding, which was independently confirmed via the actual applyRevoke/map-mutation ordering and is now covered by a regression test.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. The original optional-chain check already narrowed ledger correctly; the explicit ledger !== null guard is a clarity improvement, not a bug fix. Thanks for clarifying.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
I found four correctness issues in the new cluster skill path that can either reject supported skill configurations or install a different skill set/version than the daemon-local path. The fencing, replay authentication, exact receipt verification, and feature negotiation are thoughtfully structured, but the cluster plan still needs to preserve the existing selection, version-retention, identity, and precedence invariants before it is safe to merge.
Verification: I reviewed the exact PR head through the trusted merge whose parents are the supplied base and head. git diff --check passed, as did node --check for the three changed .mjs scripts. I could not run the focused Vitest files because this isolated checkout has no dependencies; the default pnpm store is unwritable, and the task-scoped Corepack fallback could not resolve npm (EAI_AGAIN).
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| if (source.selections.length === 0) continue | ||
| const returned = returnedSelections.get(source.sourceId) ?? new Set<string>() | ||
| if ( | ||
| source.selections.some((selection) => !returned.has(selection)) || |
There was a problem hiding this comment.
This compares installed directory leaves with the CLI selection strings, but those are deliberately different values for supported sources. resolveSkillSelections() returns cliSelections such as "Grill Me" and expectedLeaves such as "grill-me"; daemon.ts sends only the former here, while returned is populated from the root leaf. A valid selected skill with a display-style frontmatter name therefore gets published by the shim and then permanently fails this check before the durable commit. Carry expectedLeaves separately and compare the receipt against those, while still passing cliSelections to the CLI.
There was a problem hiding this comment.
Fixed in dc982e9. Cluster sources now carry CLI selections and exact expected leaves separately; the shim still receives display/frontmatter selections while coordinator receipt validation compares canonical leaves. The coordinator regression uses a display-style selection with a different expected leaf.
There was a problem hiding this comment.
Confirmed — matches the fix I found: ClusterSkillSnapshotSource now carries selections (CLI/frontmatter names) and expectedLeaves (canonical installed leaf names) as separate fields, and the coordinator's receipt validation in reconcile() checks against expectedLeaves rather than selections. The updated cluster-skill-coordinator.test.ts case (Agent Display Name selection vs. agent expected leaf) exercises exactly that divergence, which the old single-field version couldn't have caught. Approved this on the full revision already — thanks for closing it out.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. Separating CLI selections from canonical expectedLeaves fixes the receipt mismatch, and the regression test exercises the previously failing display-name/leaf-name divergence. Thanks for the fix.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| if (!entry.githubRepoId) continue | ||
| const currentEntry = { ...entry, githubRepoId: entry.githubRepoId } | ||
| try { | ||
| const acquired = await acquireGitSkillSource(currentEntry, { |
There was a problem hiding this comment.
Every cluster cold preparation resolves the configured branch/HEAD again in a fresh scratch directory. The local installer instead persists a definition-digest → resolved-commit mapping and reuses that SHA until the repository/ref definition changes; the design explicitly says moving branches must not advance on every session. The cluster ledger currently stores only installed roots, so an unchanged unpinned source will silently update after a cold host restart. Persist/reuse the Git resolution before calling acquisition, as the local path does.
There was a problem hiding this comment.
Fixed in dc982e9. The cluster ledger now persists definition-digest to resolved-commit records. Cold preparation reuses the retained SHA as the acquisition ref and rejects any mismatched result; changing the source/ref definition drops the old mapping through the same retention logic as the local installer.
There was a problem hiding this comment.
Confirmed — matches what I found: daemon.ts retains {definitionDigest, resolvedCommit} pairs in the cluster ledger (cluster-skill-ledger.ts's new gitResolutions field), reuses currentGitResolutions/gitResolutionDigest from the local installer to pin cold-start acquisitions to the retained SHA, verifies the re-resolution actually lands on that commit, and lets a changed definition digest naturally drop the stale mapping since currentGitResolutions filters by the current set of configured entries. Already covered in my APPROVE on the full revision.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. The durable {definitionDigest, resolvedCommit} mapping now pins unchanged moving refs across cold preparations, verifies reacquisition against the retained SHA, and naturally expires when the repository/ref identity changes. Thanks for addressing this.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| throw new Error('cluster skill reconciliation targets a stale sandbox launch') | ||
| } | ||
| const operationId = randomUUID() | ||
| const sources = [...input.sources].sort((a, b) => a.sourceId.localeCompare(b.sourceId)) |
There was a problem hiding this comment.
Lexically sorting by sourceId changes collision precedence before reconcileSkillBundles, whose de-duplication intentionally makes the later candidate win. With the current prefixes this orders agent → dream → managed, so a managed skill overrides an accepted Dream skill of the same name, contrary to the documented accepted-local > managed > Git rule. The caller currently builds managed → dream → Git, so simply removing this sort is not sufficient either; construct the explicit Git → managed → Dream plan (with deterministic order within each class) and hash/upload that same order.
There was a problem hiding this comment.
Fixed in dc982e9. Coordinator no longer lexically reorders sources. The daemon constructs one deterministic precedence plan: configured Git order, managed sorted by key, then accepted Dream sorted by key, so later de-duplication preserves Git < managed < Dream precedence.
There was a problem hiding this comment.
Confirmed — this matches what I verified: the sourceId.localeCompare sort in ClusterSkillCoordinator.reconcile is gone, and daemon.ts now assembles sources as [...gitSources, ...managed sorted by key, ...dreamed sorted by key], giving one deterministic Git < managed < Dream precedence plan. The updated coordinator test asserting order round-trips unsorted covers it. Already folded into my APPROVE on the full revision.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. Preserving the daemon’s explicit Git → managed → Dream ordering restores the intended later-wins precedence, while sorting within managed and Dream keeps the plan deterministic. Thanks for the fix.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| currentEntry.skills | ||
| ) | ||
| sources.push({ | ||
| sourceId: `agent:${currentEntry.githubRepoId}:${acquired.resolvedCommit}`, |
There was a problem hiding this comment.
This is not a unique source identity. Two valid source rows may point at the same GitHub repository and resolved commit while having different names, subdirectories, or selections. They then receive the same sourceId, and the coordinator rejects the entire preparation as duplicate before the shim can install either source. The daemon-local path keys Git inputs by their distinct definition/order; include an equivalent definition identity (and, if needed, the entry position) here so separately configured sources do not collide.
There was a problem hiding this comment.
Fixed in dc982e9. Git source IDs now include the configuration index, definition digest, and resolved commit. Two entries targeting the same repo/commit remain distinct even with different names/subdirectories/selections.
There was a problem hiding this comment.
Confirmed — matches what I found: sourceId is now `agent:${index}:${definitionDigest}:${resolvedCommit}`, so two configured entries pointing at the same repo/commit stay distinct regardless of name/subdirectory/selection differences. I noted one minor non-blocking follow-up on this in my approval: tying identity to index means reordering unrelated agent.skills entries will churn/reinstall git sources whose content didn't actually change — not a correctness issue, just avoidable install churn worth a future cleanup. Already folded into my APPROVE on the full revision.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. Including the configuration index alongside the definition digest and resolved commit makes otherwise identical repo/commit entries distinct, resolving the collision. Index-based churn on reordering is non-blocking. Thanks.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
Both issues I raised on the prior revision are fixed correctly here:
duty-coordinator.ts:projectedFences(groupIds)now snapshots{groupId, term}synchronously before the async revoke loop runs, soapplyRevokedeleting the map entries no longer races the fence-revocation loop. Covered by the newdaemon-duty-fence.test.tscase asserting both groups in a multi-group self-fence getrevokeDutyWriteFencecalls.skill-install-ledger.ts:171: now uses an explicitledger !== null &&guard, which correctly narrows the type beforepublicationMacValid(ledger, ...).
I spot-checked one of the other reviewer's findings against this revision's code and it holds: daemon.ts builds Git-source sourceId as `agent:${githubRepoId}:${resolvedCommit}` (no subdirectory/selection/ref discriminator), and ClusterSkillCoordinator.reconcile rejects any duplicate sourceId outright (cluster skill sources contain duplicate identities) — so two configured skill entries pointing at the same repo+commit with different subpaths/selections will hard-fail reconciliation rather than install both. That's a real regression path for a plausible config. I didn't independently re-derive the other three findings (Git ref re-resolution, source-sort precedence, receipt-name comparison) given the scope of this pass, but the one I checked was accurate, so I'm deferring to that review's REQUEST_CHANGES rather than re-litigating them.
Net: my own findings are resolved, but this revision still has open blocking issues from the parallel review.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
The synchronization correctly fixes the multi-group duty-fence revocation race and makes the publication-ledger null narrowing explicit. However, the exact f037187c head leaves four cluster-skill correctness blockers unchanged: selected Git skills are validated against CLI names rather than installed leaves, moving Git refs are resolved afresh on each cold preparation, lexical source sorting violates the required Git → managed → accepted-Dream precedence, and distinct configurations of the same repository/commit collide on one sourceId.
I inspected the exact new head through GitHub because this review generation has no trusted local checkout. The synchronization changes only duty-coordinator.ts, skill-install-ledger.ts, and the duty-fence test, so local execution was not available for this pass.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| if (source.selections.length === 0) continue | ||
| const returned = returnedSelections.get(source.sourceId) ?? new Set<string>() | ||
| if ( | ||
| source.selections.some((selection) => !returned.has(selection)) || |
There was a problem hiding this comment.
Still blocking on this head: returned contains installed directory leaves, but source.selections contains the frontmatter names passed to the CLI. For a supported source selected as grill-me whose manifest says name: Grill Me, the shim publishes leaf grill-me and this post-publication check rejects it because it expects Grill Me. Carry resolveSkillSelections(...).expectedLeaves separately for this receipt check while passing only cliSelections to the shim CLI.
| if (!entry.githubRepoId) continue | ||
| const currentEntry = { ...entry, githubRepoId: entry.githubRepoId } | ||
| try { | ||
| const acquired = await acquireGitSkillSource(currentEntry, { |
There was a problem hiding this comment.
Still blocking on this head: cluster preparation always acquires the configured branch/HEAD into fresh scratch, so every cold host preparation may advance a moving source. The local installer persists a definition-digest → resolved-commit mapping and reacquires that SHA until repository/ref changes, which is also the behavior specified in shared-skills.md §5. The cluster durable state needs the equivalent retained resolution before this call.
| throw new Error('cluster skill reconciliation targets a stale sandbox launch') | ||
| } | ||
| const operationId = randomUUID() | ||
| const sources = [...input.sources].sort((a, b) => a.sourceId.localeCompare(b.sourceId)) |
There was a problem hiding this comment.
Still blocking on this head: lexical sourceId order becomes agent → dream → managed, and reconcileSkillBundles intentionally gives a same-path collision to the later candidate. That makes managed override accepted Dream, contrary to the documented accepted-Dream > managed > Git precedence. The caller itself currently builds managed → Dream → Git, so establish the explicit Git → managed → Dream plan here or at the caller, then hash and upload that same deterministic order.
There was a problem hiding this comment.
All previously raised blocking issues are addressed in this revision (dc982e96), verified against the diff from f037187c:
- Receipt validation vs. CLI names:
cluster-skill-coordinator.tsnow validates against a distinctexpectedLeavesfield (fromresolveSkillSelections) instead of conflating it withselections(CLI display names). - Source precedence broken by sorting: the
sourceId.localeComparesort inClusterSkillCoordinator.reconcileis removed;daemon.tsnow builds the list explicitly as git sources first, then managed/dream sorted by key — preserving intended override precedence. Covered by an updated coordinator test that asserts source order round-trips unsorted. - Moving Git refs re-resolved every cold start:
daemon.tsnow reuses the existingcurrentGitResolutions/gitResolutionDigestfingerprinting (previously local-install-only, now exported) to pin each configured Git source to its previously-resolved commit across preparations, with an explicit check that re-resolution still lands on the retained commit. sourceIdcollisions for distinct same-repo/commit configs: Git source ids are nowagent:${index}:${definitionDigest}:${resolvedCommit}, disambiguated per configured entry rather than just repo+commit.
My own two findings from the prior revision (duty-fence batch-revoke race, ledger null-narrowing) remain fixed and regression-tested.
One minor, non-blocking observation: the new agent:${index}:... sourceId ties git-source identity to the entry's position in agent.skills, so reordering unrelated skill entries will churn/reinstall unrelated git sources on the next reconcile even though their content didn't change. Not a correctness issue, just avoidable install churn — fine to leave as a follow-up.
Approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved at dc982e96d06ba3b6c85951270a23d806c367d134.
This revision resolves the four remaining correctness findings: exact receipts are checked against canonical installed leaves while the CLI still receives frontmatter names; source publication order is explicitly Git → managed → accepted Dream; Git source identities are unique even when definitions share a repository and commit; and moving-ref resolutions are retained in the durable cluster ledger and reused for reacquisition. The earlier duty-fence batch race is also still correctly fixed.
Non-blocking compatibility warning: currentGitResolutions(configuredGitSources, ...) runs before the per-entry try and calls gitResolutionDigest on rolling-compatible historical entries. A malformed legacy source/ref/subdirectory that still carries githubRepoId can therefore throw before the existing omit-and-warn path. Strictly admitting entries first, as the local installer does, would preserve graceful rolling-upgrade behavior; per the project's active-development policy, I am not blocking on this compatibility edge.
git diff --check passes. I could not run the daemon typecheck or focused tests locally because this checkout has no installed dependencies and pnpm's automatic install could not open its configured store database; the submitted verification reports those gates passing.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| Boolean(entry.githubRepoId) | ||
| ) | ||
| const resolutionsByDefinition = new Map( | ||
| currentGitResolutions(configuredGitSources, prior?.ledger.gitResolutions ?? []).map((resolution) => [ |
There was a problem hiding this comment.
Non-blocking compatibility note: this helper calls gitResolutionDigest for every githubRepoId-bearing compatible entry before the per-entry try. A historical malformed source/ref/subdirectory can therefore throw here and block cluster startup instead of being omitted with a warning. Strict-parse the entries first (like installSkills) or move digesting under the existing per-entry catch when convenient.
There was a problem hiding this comment.
Fixed in ea9c242. Cluster preparation now strict-parses historical Git entries before resolution retention; invalid entries are warned and omitted, and retained-resolution and digest work only receives admitted entries with their original configuration index.
There was a problem hiding this comment.
Confirmed — matches what I found: configuredGitSources now strict-parses each historical Git entry via AgentSkillEntrySchema.safeParse before it ever reaches gitResolutionDigest/resolution retention, warns and omits anything that fails, and preserves the original agent.skills index for admitted entries so the sourceId/retained-resolution scheme stays consistent. Already covered in my APPROVE on this revision.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Confirmed. Strict parsing now keeps malformed historical entries on the intended warn-and-omit path, while preserving original indices for admitted sources. This cleanly resolves the compatibility edge—thanks.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
This revision (ea9c2429) makes one targeted change vs. dc982e96: configuredGitSources in daemon.ts now re-validates each agent.skills entry with githubRepoId against AgentSkillEntrySchema.safeParse before treating it as a Git source, logging and omitting (rather than admitting) any entry that fails current schema validation or loses its githubRepoId after parsing. This closes the compatibility gap the other reviewer flagged non-blocking on the prior revision — a malformed historical entry could previously slip past the loose Boolean(entry.githubRepoId) predicate and reach gitResolutionDigest/acquireGitSkillSource unvalidated. The index/entry pairing used for sourceId and resolution retention is preserved correctly through the change.
No new concerns. Approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved at ea9c2429ad17d8bada2da22fe34514e321f143e4.
This one-file follow-up closes the remaining non-blocking rolling-compatibility edge from the prior revision: cluster preparation now validates each CompatibleAgentSkillEntry with the current AgentSkillEntry schema before passing it to currentGitResolutions / gitResolutionDigest. Malformed historical entries are therefore omitted with a warning instead of throwing before the per-source degradation path. Keeping each entry's original configuration index preserves the distinct Git source identities introduced in dc982e96.
The previously approved invariants remain intact: CLI selections and canonical receipt leaves are separate, source precedence is Git → managed → accepted Dream, moving Git refs retain their resolved commit, and same-repository entries have distinct identities. The exact commit and its sole parent/diff were inspected through GitHub read-only endpoints; no trusted local checkout was available, so local execution was skipped.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
This revision (8642b11d) is a narrow "restore test compatibility" follow-up on top of ea9c2429, all pre-release schema/test cleanup — no behavioral concerns:
local-store.ts: folds thereplayKeycolumn directly into the basecluster_skill_journalCREATE (already present there), drops the now-redundantALTER TABLE ... ADD COLUMN replayKeymigration step, and revertsSCHEMA_VERSION15 → 14. Verified the self-check invariant still holds: 13 migration steps ===SCHEMA_VERSION - 1. Since this table was introduced within this same unreleased PR, collapsing the mid-development migration is safe — nothing has shipped at v15.postgres-dialect.ts: registers the new cluster-skill-journal/ledger columns (desiredHash,groupId,priorRevision,replayKey,resultLedger,workspaceIncarnation) in the canonical column list, matching the queries already added inlocal-store.ts.daemon.ts:reconcileClusterSkillsnow computesdreamedfirst and early-returns when there's no client and no configured/managed/dream skill sources at all, instead of unconditionally throwing on missing plane/duty/daemonId for agents with nothing to install. Consistent with the existingsupportRequiredgating pattern later in the same function.k8s-driver.test.ts: mock/assertion updates to match the driver's actual{claim, created}return shape and theskillssandbox grant, both already present in the driver code from earlier commits.
Nothing new to flag. Approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved at 8642b11d056556b2fc1923b47440a58e9db07cd8.
The PostgreSQL canonical-column additions cover the new cluster ledger/journal row shapes, the schema version now correctly treats the new tables (including replayKey) as one CREATE-block addition, and the k8s driver fixtures match the current claim/grant contracts. The prior cluster-skill correctness fixes remain intact.
One non-blocking runtime-image compatibility warning: the new !client && no configured sources return happens before reading the durable cluster ledger. Because a SandboxClaim and its volume survive runtime-image changes, rolling back to an image without the skills capability after removing the last configured skill can leave prior receipt-owned skill roots active while preparation returns successfully. The existing supportRequired(... priorRoots) check was specifically preserving that cleanup/fail-closed requirement. When convenient, keep the empty fast path only after establishing that priorRoots is also empty (or otherwise verify the preserved volume has nothing to remove). Per the project's active-development compatibility policy, I am not blocking this revision on that rollback edge.
The exact commit, parent, and four-file diff were inspected through GitHub read-only endpoints. No trusted local checkout was available, so local execution was skipped.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
This revision (558a1ffd) touches only two test files, no production code:
cluster-acp-e2e.test.ts: updates itsensureClaimfake to return{ claim, created: true }, matching the driver's actual return shape (already fixed in the other k8s-driver test in the prior revision).direct-connect-credentials.test.ts: updates the closed-setRUNTIME_GRANTSassertion to include'skills', which is already present in production (sandbox-identity.ts).
Both are the last stragglers of the same test-fixture alignment from the previous revision — verified RUNTIME_GRANTS in sandbox-identity.ts already includes 'skills', so this just catches up assertions to existing behavior. The non-blocking rollback/cleanup observation from the other reviewer on 8642b11d is unaffected since no production logic changed here.
Approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved at 558a1ffdbaee03ff2b43e3bed568b3dbcd086c79.
This revision changes only two test fixtures: the cluster ACP E2E fake now returns the current { claim, created } result from ensureClaim, and the direct-connect closed grant-set assertion includes the already-reviewed skills capability. Both changes align the tests with production contracts introduced earlier in the PR; no production behavior or previously reviewed cluster-skill invariants changed.
The exact commit, parent, and two-file diff were inspected through GitHub read-only endpoints. No trusted local checkout was available, so local execution was skipped.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
Approved at ff2addc03976c11a297321abf3ef354f647ebb8f.
This one-line follow-up removes the premature !client && no desired sources return. Cluster preparation once again reads the ledger and includes priorRoots in clusterSkillSupportRequired before allowing an incapable shim to proceed, so a preserved claim/PVC cannot silently start with stale receipt-owned skills after the desired set becomes empty. Empty agents with no prior roots still take the existing safe return after that check.
All previously approved receipt, precedence, Git identity/retention, schema, PostgreSQL, and test-fixture changes remain intact. The exact commit, parent, and one-line diff were inspected through GitHub read-only endpoints; no trusted local checkout was available, so local execution was skipped.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
This revision (ff2addc0) is a single-line revert in daemon.ts: removes the if (!client && agent.skills.length === 0 && ...) return early-return that 8642b11d added to reconcileClusterSkills. That's exactly the fix for the other reviewer's non-blocking observation on 8642b11d — the early return, by returning before prior (the durable cluster-skill ledger) was even read, could skip cleanup of previously-installed skill roots on a preserved volume for an agent that currently has zero configured sources (e.g. after a runtime-image rollback or shim swap that dropped skill support).
Confirmed reconcileClusterSkills is only ever called from inside the if (this.k8sPlane) branch of runAgentWorkspacePreparation (daemon.ts:3305-3312), so plane is always truthy at call time — the restored unconditional if (!plane || !workspaceIncarnation || ...) throw gate doesn't reintroduce any risk for self-hosted (non-cluster) daemons; it only affects already-cluster-mode agents, correctly forcing the full prior/supportRequired cleanup path to run instead of bypassing it. No test files changed alongside this, consistent with the removed branch never having been load-bearing for existing coverage.
Approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved at 88cf63b9be13f0d2ec6062441a498ff6dfca9b54.
This revision restores empty-cluster compatibility without reopening the stale-root gap. Production workspace preparation enters through withSandbox, which records the current launch/claim UID before reconcileClusterSkills; whenever that workspace incarnation exists, the daemon reads the durable ledger and includes priorRoots in supportRequired before allowing an incapable shim to return. The new fallback applies only when there is no plane/incarnation, no skills client, and no desired sources, where there is no identified cluster workspace to reconcile.
The later authority check still gates every actual skill mutation, and all previously approved cluster-skill invariants remain unchanged. The exact commit, parent, one-file diff, and relevant lifecycle code were inspected through GitHub read-only endpoints. No trusted local checkout was available, so local execution was skipped.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
There was a problem hiding this comment.
This revision (88cf63b9) refines the guard removed in ff2addc0 rather than reverting it — a more precise fix than the original 8642b11d attempt:
- The empty-agent bypass now gates on
!plane || !workspaceIncarnationspecifically (i.e. no resolvable workspace identity yet — a genuinely new pod that can't have any preserved volume state), not on!client. SinceworkspaceIncarnationis exactly the key needed to look upprior(the durable cluster-skill ledger), this is the correct condition: if the incarnation is known, a volume could already carry prior roots, so the code now always falls through to computeprior/supportRequiredin that case — closing the preserved-volume cleanup gap the other reviewer flagged, without reintroducing spurious throws for a brand-new pod with nothing configured and no client. - The
shimGeneration/duty/daemonIdauthority check moved to right before the actual staging/install work (mkdtemponward), after the!client/!skillsAgentId/supportRequiredshort-circuits — so a skill client resolved with real work to do still requires full duty authority before touching the sandbox, but the "nothing to install" paths aren't blocked on it.
Traced through the !client, !skillsAgentId, and final authority branches — all consistent, prior/supportRequired is always computed whenever a workspace incarnation exists, so the cleanup-skip risk from 8642b11d doesn't resurface. No test changes accompany this, consistent with it being a targeted logic refinement rather than new surface area.
Approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
Round 7: one file, three failures — `shim-skill-handler.test.ts`, which #1545 added to main while this branch was converging. It is the sandbox-pod plane like every other shim suite, so it joins that group; the comment now says a new suite over that plane belongs there, since the ones absent from the list do pass on Windows. Merges main, which brings #1535 — the same win32-only EACCES/EBUSY/EPERM rename retry for workspace directory swaps that this branch added for the memory publish, arrived at independently. The memory-side comment now points at it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Verification
pnpm --filter @agentconnect.md/daemon typecheckpnpm --filter @agentconnect.md/daemon buildpackages/daemon/src+packages/daemon/test) and Prettier checksagentconnect-runtime-sandbox:cluster-skills-prnode scripts/verify-runtime-image.mjs agentconnect-runtime-sandbox:cluster-skills-prdocs/superpowers/**planning artifacts are intentionally not included.