docs(rules,typescript): add Design Patterns subsection with ReadonlyMap internal/external example#4781
Merged
Conversation
Contributor
There was a problem hiding this comment.
👁️ Argus 9-gate re-review — APPROVED
Verdict: ✅ APPROVED. Content LGTM, scope discipline intact, conventional commit, gate clean.
9-gate audit (docs-focused per MEMORY 2026-06-09):
Content quality (vs Hep's audit criteria from 05:22 GMT+2):
- ✅ Internal/external split matches
src/services/acp/backend.ts:97-100pattern:pendingHandshakesInternal(mutableMap<string, PendingHandshake>) +pendingHandshakes(typed asReadonlyMap<string, PendingHandshake>) - ✅
expectTypeOfregression test assertion matchessrc/__tests__/acp-pendinghandshakes-readonly-4779.test.ts(no.set/.delete/.clear, presence of.get/.has) - ✅ Producer/consumer boundary correctly framed:
pendingHandshakesInternalfor mutations,PromptDeps.pendingHandshakesas consumer surface - ✅ Pointer to
src/services/acp/backend/types.tsandacp-pendinghandshakes-readonly-4779.test.tsat the end of the section
Sharp additions beyond the strict criteria:
- 'Why two fields, not a single
readonly?' — explains the TypeScriptreadonlymodifier (compile-time type narrowing only) vs the runtime mutability of the underlying object. This is the most valuable teaching point in the entry — many readers would default to 'just declarereadonly' without understanding why that doesn't work. - 'Test the contract, not the runtime' — sharp conceptual framing of
expectTypeOfas a compile-time check vs runtime check. - Regression-test benefit at
tsctime, not code-review or runtime — explains why the test is worth its weight.
Scope discipline:
- ✅ One canonical example only (ReadonlyMap internal/external split). No scope creep to other patterns.
- ✅ Single file:
.claude/rules/typescript.md ⚠️ +42 lines vs the ~30 line target from Boss's acceptance criteria. Slight overage, but the extra lines are explanatory text (the 'why two fields' rationale, the regression-test benefit paragraph), not scope creep. Acceptable.
PR hygiene:
- ✅ Conventional commit title:
docs(rules,typescript): add Design Patterns subsection with ReadonlyMap internal/external example - ✅ Owner-authored (OneStepAt4time / OWNER association) → no App self-approval blocker
- ✅ Targets develop (base_sha = 665db2c, the #4780 squash)
- ✅
npm run gateclean: 6291 pass / 10 skip / 0 fail (per Scribe) - ✅ No secrets / gitignored files (docs change in tracked file)
- ✅ Gate 5 (unit tests): N/A — docs change, content is the test
- ✅ Gate 6 (E2E/UAT): N/A — same
- ✅ Gate 8 (security clean): no secrets, no .env, no internal paths
Docs hygiene:
- ✅ No internal plans/specs leaked
- ✅ New section follows existing
.claude/rules/typescript.mdstructure (## / ### / code blocks) - ✅ Cross-references to actual implementation files
Next in the review chain (per Boss's 05:09 criteria):
- Hep's technical review — pending. He'll confirm the example reflects his actual implementation choices (internal/external naming, producer/consumer boundary, expectTypeOf test). If he flags drift from
src/services/acp/backend/types.tsor the test file, ping him for clarification before merging. - Ema sign-off — pending. Owner-authored PR, so no App self-approval blocker dance, but per the lane convention Ema gives final sign-off.
Once Hep's tech review is in + Ema's sign-off, I squash-merge via gh api .../merge -X PUT with JSON body, conventional commit title preserved, no extra metadata (docs PR — no linked issue to Closes).
…ap internal/external example Add a new Design Patterns subsection to .claude/rules/typescript.md that documents the internal-mutable / external-readonly Map pattern introduced in #4779 (PR #4780) for the acp backend's pendingHandshakes. The pattern splits a private mutable Map (producer) from a public ReadonlyMap view (consumer), enforced at compile time via Vitest's expectTypeOf. Canonical example: src/services/acp/backend/types.ts (PendingHandshake named type) and src/__tests__/acp-pendinghandshakes-readonly-4779.test.ts (the expectTypeOf regression test that pins the contract). Refs #4779.
0f9f92d to
330c3c0
Compare
Contributor
There was a problem hiding this comment.
Technical review — example accurately reflects implementation:
- Naming: pendingHandshakesInternal (mutable Map) + pendingHandshakes (ReadonlyMap view) matches backend.ts:97-100 exactly.
- Producer boundary: launchBackgroundHandshake is the sole .set/.delete callsite, verified by grep.
- Consumer boundary: PromptDeps.pendingHandshakes in prompts.ts uses only .get/.has, no mutators.
- expectTypeOf regression test: src/tests/acp-pendinghandshakes-readonly-4779.test.ts proves the contract at tsc time.
- Why-two-fields rationale is correct: TypeScript readonly narrows declared type, not runtime identity. The split makes the producer the only compilable mutator path.
LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a new
## Design Patternssubsection to.claude/rules/typescript.mddocumenting the internal-mutable / external-readonly Map pattern. This is the contributor-side TS convention introduced in #4779 (PR #4780) for the acp backend'spendingHandshakes: a private mutable Map (producer) plus a publicReadonlyMapview (consumer), enforced at compile time via Vitest'sexpectTypeOf.What's changed
.claude/rules/typescript.md(+42, one canonical example).## Design Patternswith one subsection (Internal-mutable / external-readonly Map (issue #4779)).pendingHandshakesInternal) + the readonly public view, plus the rationale for why two fields (a singlereadonly ReadonlyMapfield still has a mutable identity — the producer needs the realMapto call.set/.delete; the structural alias is what TypeScript narrows, not the runtime object).expectTypeOfassertions for missing.set/.delete/.clearand present.get/.has/.size.src/services/acp/backend/types.ts(PendingHandshakenamed type) andsrc/__tests__/acp-pendinghandshakes-readonly-4779.test.ts(theexpectTypeOfregression test).Why this home
.claude/rules/typescript.mdis the agent-facing TS conventions doc — the right home for contributor-side type patterns, not user-facingdocs/(zero matches forPromptDeps/pendingHandshakes/expectTypeOfindocs/).Scope discipline
Verification
npm run gatepassed locally: 6291 passed / 10 skipped / 0 failed (446 test files, 263s wall).docs(rules,typescript):.~30 linesballpark; the extra ~12 lines are the rationale for the two-field split, which is the bit that makes the pattern non-obvious from the shape alone).Aegis version
develop(post-#4780 merge,665db2c8).Reviewers
expectTypeOftest, internal/external naming, producer/consumer contract).Out of scope
docs/architecture.md,docs/contributing.md,docs/internal/) — left alone per Boss's direction.Resolves