Conversation
## Summary - Added writeTeamContextToMailbox() in spawnInProcess.ts that writes a [TEAM CONTEXT] message to the new teammate's inbox immediately after task registration, using the existing writeToMailbox() infrastructure - Context includes: team name, agent's role, team lead name, peer teammates with agentTypes, config file path, and shared goal - Updated TEAMMATE_SYSTEM_PROMPT_ADDENDUM to point agents to the config file for live roster discovery across turns ## Impact - user-facing impact: teammates receive team roster, leader identity, and config path on first idle poll without the leader manually briefing each agent; enables peer-to-peer coordination without a central proxy - developer/maintainer impact: uses the existing mailbox system (same channel agents already poll) rather than mutating the task prompt string ## Testing - [x] `bun run build` - [x] `bun run smoke` - [ ] focused tests: requires a live swarm to validate inbox delivery; writeToMailbox() is already tested via the mailbox test suite ## Notes - provider/model path tested: n/a (prompt construction is model-agnostic) - screenshots attached (if UI changed): n/a - follow-up work or known limitations: pane-backed (tmux/iTerm2) teammates use a different spawn path and may need the same treatment https://claude.ai/code/session_01D7kprMn4c66a5WrZscF7rv
Move team context injection to spawnMultiAgent.ts for both pane-based and in-process teammates. Add buildTeamContextBlock() helper to teamHelpers.ts that formats a structured [TEAM CONTEXT] block with team name, agent role, lead, teammates, config path, and shared goal. - teamHelpers.ts: add buildTeamContextBlock(agentName, agentType, teamFile) - spawnMultiAgent.ts: prepend team context block to writeToMailbox() text for split-pane and separate-window paths, and prepend to prompt for in-process path - spawnInProcess.ts: remove incorrect standalone mailbox write; context is now injected upstream by spawnMultiAgent - teamHelpers.test.ts: 8 unit tests covering all block fields, exclusions, and optional-field omissions https://claude.ai/code/session_01D7kprMn4c66a5WrZscF7rv
|
This is another from my list of the initial source analysis. All my PRs are related to that research. Im trying to hold off until after the rename on further PRs but I saw the issue spring up. |
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for the PR. The team-context addition is a useful improvement, and I like the direction of centralizing the formatting in buildTeamContextBlock().
I did find one blocking issue before this can be merged:
src/tools/shared/spawnMultiAgent.ts:956-960, 985, 1069-1076
InhandleSpawnInProcess(),promptWithContextis used in bothconfig.promptandstartInProcessTeammate({ prompt: ... })beforeteamContext/promptWithContextare declared later in the function. Sinceconstbindings are in the temporal dead zone until initialization, this will fail at runtime withReferenceError: Cannot access 'promptWithContext' before initializationwhenever the in-process spawn path is used.
Other than that, I didn’t find any additional high-confidence regressions in the PR head I reviewed.
Happy to take another look once that’s fixed.
Updated based on feedback. Moved teamContext up to fix regression. Ill fix my CI to avoid further blunders.
---
Thanks for the PR. The team-context addition is a useful improvement, and I like the direction of centralizing the formatting in buildTeamContextBlock().
I did find one blocking issue before this can be merged:
src/tools/shared/spawnMultiAgent.ts:956-960, 985, 1069-1076
In handleSpawnInProcess(), promptWithContext is used in both config.prompt and startInProcessTeammate({ prompt: ... }) before teamContext / promptWithContext are declared later in the function. Since const bindings are in the temporal dead zone until initialization, this will fail at runtime with ReferenceError: Cannot access 'promptWithContext' before initialization whenever the in-process spawn path is used.
Other than that, I didn’t find any additional high-confidence regressions in the PR head I reviewed.
|
please rebase to latest main |
|
please rebase to main branch to fix the broken tests |
|
saw it. let me recheck again |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head a37c3c3b9bdac230f3297dac52391a04375ddc1e against current origin/main 72e6a945fe1311a0b9eea1033703a74dc6cdbc18.
The earlier TDZ issue is fixed on this revision, but I still can't approve because moving the team-file registration earlier introduced a new in-process regression.
Current blocker:
-
src/tools/shared/spawnMultiAgent.ts:956-971now appends the teammate toteamFile.membersand writes the file beforespawnInProcessTeammate()is attempted at:989. If that spawn then fails, the function throws at:991-992but leaves the new member persisted in the team file even though no teammate actually exists.Direct repro on this head:
- mock
spawnInProcessTeammate()to return{ success: false, error: 'spawn failed' } - call
spawnTeammate()through the realhandleSpawnInProcess()path - function throws
spawn failed - captured
writeTeamFileAsync()payload already contains the newworker@teammember
Baseline on current
origin/mainfor the same repro:- function throws
spawn failed writeTeamFileAsync()is never called (nullin the repro)
This is a real behavioral regression: a failed in-process spawn now leaves a ghost teammate in the team roster, which can affect later roster discovery / unique-name generation and make the persisted team config disagree with reality.
- mock
What I verified on this head:
- direct failure-path repro above on latest head
- same failure-path repro on current
origin/mainfor baseline bun test src/utils/swarm/teamHelpers.test.ts-> 8 passbun run build-> successbun run smoke-> success
I didn't find another high-confidence blocker beyond that failure-path regression, but I wouldn't merge this until the team file write is made atomic with successful in-process spawn (or rolled back on failure).
|
I will rebase and resubmit following the feedback, thanks! |
If spawnInProcessTeammate() fails, the team file was already updated, leaving a ghost entry that corrupts name generation and persisted config. Move writeTeamFileAsync() to after the success check so the file is only written when the spawn actually completes. Fixes atomicity regression flagged in PR Gitlawb#410 review by Vasanthdev2004. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
Covers the failure-path regression flagged in PR Gitlawb#410: - Team file is NOT written when spawnInProcessTeammate fails - Team file IS written exactly once on success - writeTeamFileAsync is always called AFTER spawnInProcessTeammate (no TOCTOU) - spawnTeammate re-throws spawn errors verbatim - startInProcessTeammate is not called on spawn failure - Written team file contains the new member on success These tests would have caught the ghost-teammate regression before review. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
…e I/O Replace the wholesale mock of teamHelpers.js (which bled into teamHelpers.test.ts across parallel bun workers) with a real tmpdir approach: redirect CLAUDE_CONFIG_DIR to a per-test temp directory so file I/O uses the real helpers without polluting the shared module registry. The TOCTOU test now captures the on-disk team file state at the exact moment spawnInProcessTeammate is called, giving a more honest assertion than call-order tracking. Pre-existing suite failures (preconnectAnthropicApi, TextEntryDialog, wizard step) are unrelated and unchanged. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
Three layers of defence against the ghost-member regression class: 1. pr-intent-scan: new findFileOrderingFindings() reads the current state of modified TypeScript files and flags any function where await write*FileAsync() precedes await spawn*Teammate() in the same scope. This would have caught PR Gitlawb#410's atomicity bug as a medium finding before it reached review. Injected FileReader parameter keeps the check fully testable without disk I/O. 6 new tests added to pr-intent-scan.test.ts. 2. scripts/pre-commit.sh: git hook that (a) builds, (b) runs test files paired with staged sources, (c) always runs pr-intent-scan tests, (d) runs the ordering scan on the staged diff. Install with: cp scripts/pre-commit.sh .git/hooks/pre-commit && chmod +x .git/hooks/pre-commit 3. .github/workflows/pr-checks.yml: adds bun run typecheck as a continue-on-error step so type regressions are surfaced on every PR without blocking the suite while the pre-existing baseline of errors exists. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
…I/O failure Two issues surfaced by agent review of the full PR: 1. False positive in findFileOrderingFindings: regex patterns matched commented-out code (lines starting with //). Added !/^\s*\/\//.test(text) guard to both writeLines and spawnLines filters. Added test to confirm commented-out write+spawn lines are not flagged. 2. Missing test coverage: no test for writeTeamFileAsync throwing after a successful spawn (e.g. disk full, permission denied). Added test that blocks the team subdir with a regular file to force an I/O error, then asserts the error propagates to the caller and no ghost member is written. Concurrent-spawn read-modify-write race and setAppState-after-write ordering are pre-existing architectural issues outside this PR's scope; noted for follow-up. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
Claude/improve ci process u aj xg
…type 1. scripts/pre-commit.sh: consolidate the git diff | bun run ... invocation onto a single line. The backslash line-continuation was valid bash but ambiguous in a pipe context and could be misread as a literal \n argument by some shell environments. `|| true` replaces the multi-line fallback block. 2. src/tools/shared/spawnMultiAgent.inProcess.test.ts: extract TeamMember type and widen readTeamFile() return type to include backendType?: string. The previous narrow type caused a TypeScript error when asserting member?.backendType, which would surface as noise in the new typecheck CI step. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
fix: address PR review — flatten pre-commit pipe, widen readTeamFile …
Two gaps from PR review: 1. Smoke check missing: bun run smoke (build + version) is a CI requirement but was absent from the hook. Added as step 2, after staged-file collection and before tests. 2. Test discovery too narrow: the hook only matched foo.test.ts siblings. Added a directory glob (find -name "*stem*.test.*") so that staging spawnMultiAgent.ts also picks up spawnMultiAgent.inProcess.test.ts and any other variant test files in the same directory. Keeps the two explicit fallbacks (direct sibling, __tests__/) and the always-run anchor (pr-intent-scan.test.ts). Also fixes the install comment: replaced literal \n with printf to avoid the same line-continuation ambiguity flagged in the previous review. https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
After all three checks run (smoke, tests, scan), the hook now prints a structured summary table: ╔══════════════════════════════════════════════╗ ║ pre-commit check summary ║ ╠══════════════════════════════════════════════╣ ║ smoke passed ║ ║ tests passed (2 file(s)) ║ ║ scan clean ║ ╚══════════════════════════════════════════════╝ This gives a consistent, reviewable snapshot on every commit: - smoke/tests show passed/FAILED so failures are unambiguous - scan shows finding count and HIGH count (HIGH blocks the commit; medium findings are advisory and shown but do not block) - blocked commits print a clear reason before exit 1 https://claude.ai/code/session_01Mv4RRXGzaVRDbLqtpTLJzk
Claude/improve ci process u aj xg
|
Ive updated with recommended fixes, tests, and a pre-commit.sh hook I can reference from any environment to get consistent CI. If I need to split it into its own PR, I can. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the current head ad9c5bdd8f6aebe74167d432f33356cfbd0d8885 against current origin/main.
The earlier in-process spawn regressions are fixed on this head, and the new focused tests/build/smoke all pass. I still can't approve because the new pre-commit safety net has a real logic bug that makes its PR scan effectively a no-op.
-
scripts/pre-commit.sh:98plusscripts/pr-intent-scan.ts:467-489andscripts/pr-intent-scan.ts:523-526
The hook says it scans the staged diff:git diff --cached --unified=0 | bun run scripts/pr-intent-scan.ts --base HEAD
But
scripts/pr-intent-scan.tsnever reads stdin. It always recomputes its own diff withgetGitDiff(baseRef), and with--base HEADthat becomesgit diff HEAD...HEAD, i.e. an empty diff.Direct repro I verified locally on this head:
- pipe a fake diff containing
+await writeTeamFileAsync()intobun run scripts/pr-intent-scan.ts --base HEAD - output is still
PR intent scan: no suspicious additions found.
So the new pre-commit hook currently always reports the scan as clean, even when the staged changes should be flagged. That blocks approval because this PR explicitly adds the hook/scan as protection against the exact regression class discussed in the PR.
Non-blocking notes:
.github/workflows/pr-checks.yml:38-43typecheck is surfacing a large pre-existing TS baseline on both this branch and currentorigin/main; I did not treat that as a PR-specific blocker.- The two provider tests that failed in the GitHub merge run did not reproduce locally on the branch, and the focused tests for this PR passed.
What I verified on this head:
bun test src/tools/shared/spawnMultiAgent.inProcess.test.ts src/utils/swarm/teamHelpers.test.ts scripts/pr-intent-scan.test.ts-> 35 passbun test src/commands/provider/provider.test.tsx-> 10 passbun run build-> successbun run smoke-> successbun test --max-concurrency=1-> only unrelatedsrc/utils/geminiCredentials.test.tsfailures locally- direct stdin repro of the pre-commit scan issue above
Summary
What changed: Teammates now receive a structured
[TEAM CONTEXT]block prepended to their initial prompt at spawn time. A newbuildTeamContextBlock()utility inteamHelpers.tsproduces the block containing: team name, agent role/type, team lead identity, other teammates (names + types), team config file path, and shared goal. This block is injected at the three spawn paths inspawnMultiAgent.ts— split-pane (tmux), separate-window (iTerm2), and in-process — using the existing mailbox system for pane-based agents and direct prompt injection for in-process agents.Why it changed: Spawned teammates had zero knowledge of their team structure. They didn't know who the lead was, who else was on the team, or where to find the team config. This forced the lead agent to manually brief every teammate, wasting tokens and creating coordination failures when teammates couldn't discover peers or roles on their own.
Impact
buildTeamContextBlock()is a single shared function with full test coverage. Adding new fields to the team context block requires changing only one function. No new dependencies introduced.Testing
bun run build— passesbun run smoke— passesbun test src/utils/swarm/teamHelpers.test.ts— 8/8 pass, 21 assertionsNotes
Pushing now in relation to #405