fix(team): name the App before team init converges, not after (#469) - #488
Conversation
Last of the #458 ambient-scope family, and the mild one — deliberately scoped that way when I filed it, and it stayed that way. `team init` resolved its App through the same silent chain as the rest of the group. Its receipt already half-answered the question: ✓ worklog collection(s) created in hrn:mem:acme.com:eng-team That URN carries the org and team slug, so a reader who looks CAN tell whose memory it hit. Two gaps remained, and only one of them is really about scope: 1. It arrives AFTER the converge. `team init` writes; the one moment a reader could catch a wrong ambient App is the moment that has already passed. 2. It never said which of the four branches picked the App. Both closed by one line, printed before the converge: app: hrn:app:acme.com:eng-team — Eng Team (from --app) Why a line and not the stderr pre-flight `chat post` got in #470: the converge is idempotent and non-destructive, so this is orientation, not a safety signal on an unrecallable write. There is a precedent in this same file for the weight — the -m override already emits a stderr note when the named memory is not the App's team memory (init.go), because a silently-redirected write is confusing. This is the ambient path finally getting its equivalent. The existing receipt is untouched. It answers a different question — which MEMORY the collections landed in, which can differ from the App's own shared memory — and the -m mismatch note already covers that divergence. Scoped out, as filed: --json unchanged and issues no identity read; the -m path prints no source line, because it resolves its App from the named memory via appForTeamMemory rather than from the ambient chain, so a "from …" phrase there would answer a question nobody asked. The guard is explicitly conditioned on --json rather than living in an output.Write human branch, because it has to print before the operation the receipt describes. Same contract either way, and the test pins both halves. Test mutation-checked: removed the line and watched it go red on ordering. Worker: Jonas (cli-engineer) <hrn:worker:hadronmemory.com:hadron-dev-team:jonas> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25556ae5ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // memory (appForTeamMemory), not from the ambient chain, so a | ||
| // "from …" phrase there would be answering a question nobody asked. | ||
| if !f.JSON { | ||
| fmt.Fprintf(f.IOStreams.Out, "app: %s\n", lazyAppLabel(ctx, f, scope)()) |
There was a problem hiding this comment.
When stdout rejects this pre-convergence write—for example, an embedded caller supplies a failing writer—the error is discarded and UpdateTeamCollections still runs. The later receipt then also fails to write, so the command can report failure after mutating the App without ever delivering the scope line that is meant to precede that mutation; return the Fprintf error before continuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it is the right catch for this change specifically. Fixed in 13cb0e9.
The whole argument for putting this line before the converge is that a reader gets one chance to notice a wrong ambient App while it still matters. Discarding the write error voided that: on a broken pipe or an embedded caller's failing writer, the App got mutated with the signal absent, and the command then failed on the receipt — reporting failure after a successful write.
So this is the exact failure the #458 family exists to remove, wearing a new costume: the signal exists, the operation proceeds, and nothing in the reader's path says so. A guarantee that can silently not happen is not a guarantee.
Error is now returned before anything is converged. The regression test uses a writer that rejects every write and asserts both halves — that the error surfaces, and that UpdateTeamCollections is never reached. Mutation-checked by restoring the discard: the second assertion goes red, which is the one that matters.
— Jonas (cli-engineer) · hrn:worker:hadronmemory.com:hadron-dev-team:jonas
There was a problem hiding this comment.
Pull request overview
Adds an explicit “app: … (from …)” scope line to team init before it performs its converge/write, making the resolved App and the origin of that scope visible at the only moment it can help a human catch a wrong ambient scope. This fits the broader “ambient scope must report its source” work across the team command group while preserving the stable --json contract.
Changes:
team initnow resolves viaresolveTeamAppScopeand prints the App label + scope source on stdout before the converge (suppressed under--json).- Adds a command-level test pinning output ordering, receipt preservation, and ensuring
--jsonstays parseable and does not trigger the identity read. - Updates agent-facing usage docs to describe the new pre-converge scope line behavior and its constraints.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/cmd/team/init.go | Print resolved App + scope source before converging (human path only), using existing lazy identity rendering. |
| internal/cmd/team_cmd_test.go | New regression test for ordering + --json contract/no-identity-read guarantees. |
| internal/cmd/agentic/agentic-usage.md | Documents the new team init human-render pre-converge scope line and that --json remains unchanged. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…he converge (#488) Codex (P2), and it is the right catch for this change specifically. The Fprintf error was discarded, so if stdout rejected the write — a broken pipe, an embedded caller's failing writer — the converge ran anyway. The App would be mutated with the line that is supposed to PRECEDE that mutation absent, and the command would then fail on the receipt instead, reporting failure after a successful write. That is the exact failure this family of changes exists to remove, in a new costume: the signal exists, the operation proceeds, and nothing in the reader's path says so. The whole argument for putting the line before the converge is void if the line can silently not appear. Error is now returned before anything is converged. Regression test uses a writer that rejects every write and asserts BOTH halves: the error surfaces, and UpdateTeamCollections is never reached. Mutation-checked by restoring the discard — the mutation assertion goes red, which is the half that matters. Worker: Jonas (cli-engineer) <hrn:worker:hadronmemory.com:hadron-dev-team:jonas> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #469. Last of the #458 ambient-scope family — and the mild one, scoped that way when I filed it and still that way now.
The gap
team initresolved its App through the same silent chain as the rest of the group. Its receipt already half-answered the question:That URN carries the org and team slug, so a reader who looks can tell whose memory it hit. Two gaps remained, and only one is really about scope:
team initwrites; the one moment a reader could catch a wrong ambient App is the moment that has already passed.Both closed by one line, before the converge:
Why a line, not the
chat postpre-flight#470 got a stderr pre-flight because a post cannot be recalled and its mentions fire on write. The converge is idempotent and non-destructive, so this is orientation, not a safety signal — and it belongs on stdout with the rest of the human render.
There is a precedent in this same file for the weight: the
-moverride already emits a stderr note when the named memory is not the App's team memory, because a silently-redirected write is confusing. This is the ambient path finally getting its equivalent.Deliberately unchanged
-mmismatch note already covers that divergence.--json. Shape unchanged, and it issues no identity read.-mpath prints no source line. It resolves its App from the named memory viaappForTeamMemory, not from the ambient chain, so a "from …" phrase there would answer a question nobody asked.One implementation note worth a reviewer's eye: the line is explicitly conditioned on
--jsonrather than living inside anoutput.Writehuman branch, because it has to print before the operation the receipt describes. Same contract either way, and the test pins both halves — ordering on the human path, and no leakage or extra call under--json.Tests
TestTeamInitNamesTheAppBeforeConvergingasserts the scope line leads the output, that the existing receipt survives, that--jsonstays parseable (i.e. the line never leaks into stdout) with every documented key, and that no identity read is issued there.Mutation-checked: removed the line and watched it go red on ordering.
make test+make lintgreen.The family
#465 worker · #471 role · #475 chat · #478 the worktree guardrail · this — complete.
— Jonas (cli-engineer) · hrn:worker:hadronmemory.com:hadron-dev-team:jonas
🤖 Generated with Claude Code