EA-2136 sync architecture instruction files from org baseline - #194
EA-2136 sync architecture instruction files from org baseline#194bwell-dev wants to merge 1 commit into
Conversation
Updates from icanbwell/.github: - AGENTS.md: Organization-wide AI agent instruction baseline - CLAUDE.md: Symlink for Claude Code compatibility - copilot-instructions.md: Lightweight instructions for GitHub Copilot - policies/approved-tech.yaml: Technology approval list - .github/workflows: PR architecture review and commit message validation - .claude/skills: Claude Code skills (api-design-guardian, sync-to-async, etc.) - CODEOWNERS: EA ownership of instruction files Source commit: 62f8288431418749382e0b673b363841d8f8977d
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const constMatches = content.matchAll( | ||
| /const\s+(\w+)\s*=\s*['"]([a-z][a-z0-9 _-]+)['"]/gi, | ||
| ); | ||
| for (const m of constMatches) { | ||
| const constName = m[1]; | ||
| const value = m[2]; | ||
| if ( | ||
| content.includes(`useFeatureFlagValue(${constName}`) || | ||
| content.includes(`useSearchParamFeatureFlag(${constName}`) | ||
| ) { | ||
| flags.add(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 In Pattern 3 of extractFlags(), resolving a const identifier to its flag value uses a plain substring check (content.includes(useFeatureFlagValue(${constName})) with no word-boundary/delimiter, so a const name that is a prefix of another (e.g. FLAG vs FLAG_ENABLED) will match even when only the longer name is actually passed to the hook. This adds the unrelated, unused const's value as a phantom flag entry in the stale-flag report. Fix by requiring a trailing comma/paren or word boundary after the const name in the match.
Extended reasoning...
The bug: In extractFlags() (Pattern 3, around lines 86-98), when the script sees const CONST_NAME = '...' it tries to determine whether that constant is actually passed into useFeatureFlagValue(...) or useSearchParamFeatureFlag(...) by checking whether the literal string useFeatureFlagValue(${constName} (or the search-param equivalent) appears anywhere in the file's raw content:
if (
content.includes(`useFeatureFlagValue(${constName}`) ||
content.includes(`useSearchParamFeatureFlag(${constName}`)
) {
flags.add(value);
}This is a plain JavaScript substring test — there is no word boundary, comma, or closing paren required after constName. If a file defines two consts where one name is a strict prefix of another (e.g. FLAG and FLAG_ENABLED), and only FLAG_ENABLED is ever actually passed to the hook, the check for constName = 'FLAG' still returns true — because 'useFeatureFlagValue(FLAG' is a substring of 'useFeatureFlagValue(FLAG_ENABLED'.
Why nothing upstream prevents this: The regex used to capture candidate consts (/const\s+(\w+)\s*=\s*['"]([a-z][a-z0-9 _-]+)['"]/gi) has no awareness of which const is actually used where — it just grabs every const NAME = 'string' assignment in the file. The only filter applied afterward is the substring includes() check, which is exactly the check that's too loose. There's no cross-check that the match is followed by a comma, closing paren, or any other delimiter that would prove constName is the whole identifier being passed, not just a prefix of a longer identifier.
Concrete walkthrough:
- A file contains:
const FLAG = 'legacy-unused-flag'; const FLAG_ENABLED = 'real-active-flag'; const isOn = useFeatureFlagValue(FLAG_ENABLED, false);
- The capture regex finds two consts:
FLAG -> 'legacy-unused-flag'andFLAG_ENABLED -> 'real-active-flag'. - For
constName = 'FLAG_ENABLED':content.includes('useFeatureFlagValue(FLAG_ENABLED')is true (correct match) →'real-active-flag'is added. - For
constName = 'FLAG':content.includes('useFeatureFlagValue(FLAG')is ALSO true, because that literal substring exists inside'useFeatureFlagValue(FLAG_ENABLED, false)'→'legacy-unused-flag'is incorrectly added, even thoughFLAGitself is never passed to the hook anywhere in the file. - The final report now contains a phantom row for
legacy-unused-flagwith its own git-history-derived first-seen date, age, and STALE/OK status — a flag that was never actually gating anything via the hook.
Impact: Since this script's entire purpose is to drive stale-flag cleanup decisions, a phantom entry can send an engineer down a rabbit hole investigating or asking the team about a flag that isn't real (or isn't gated the way the report implies). This is a distinct defect from the tool's documented Gotchas (git history depth, renamed flags, runtime-only flags, execution time) — none of those describe a false-positive caused by name-prefix collisions.
Fix: Require a delimiter after constName in the substring check, e.g. anchor on useFeatureFlagValue(${constName}, and useFeatureFlagValue(${constName}) (or better, use a regex like new RegExp(\useFeatureFlagValue\(${constName}\s*[,)]`)`) so a prefix name can no longer accidentally match a longer name's call site.
Severity note: This is a heuristic, human-reviewed developer-audit tool (the SKILL.md explicitly tells engineers to verify each STALE flag manually before removing code), and the failure mode is a single bogus report row rather than a crash or incorrect production behavior. Also, triggering it requires a fairly specific naming collision (one flag const being a strict prefix of another, both holding flag-like string values, with only the longer one wired to the hook). All three independent verifiers that confirmed this bug rated it nit for the same reasons, and I agree — it's real and worth fixing, but not blocking.
| name: 'Commit Message Check' | ||
| on: | ||
| pull_request: | ||
| types: [opened, edited, reopened, synchronize] | ||
| push: | ||
| branches: [main, 'releases/*'] | ||
|
|
||
| jobs: | ||
| check-commit-message: | ||
| name: Check Commit Message | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check For Issue Key | ||
| uses: gsactions/commit-message-checker@v2 | ||
| with: | ||
| pattern: '^(Bump|Merge|Revert|Reapply|build\(deps(-dev)?\): bump|([A-Z]+-[0-9]+))\s+.+' | ||
| flags: 'gm' | ||
| error: 'Commit message must begin with a JIRA Issue Key: e.g. HP-123 <message>' |
There was a problem hiding this comment.
🟡 The new commit-message-checker workflow uses the "gm" regex flags, and the "m" flag makes ^ match the start of every line (not just the start of the whole commit message). Since the checker action tests the pattern against the full commit message (subject + body), a commit whose subject does NOT start with a JIRA key can still pass validation if any later body line happens to start with a ticket-like token (e.g. a body line "EA-2130 follow-up"), defeating the "must begin with a JIRA issue key" rule this workflow exists to enforce. Dropping the "m" flag (or setting excludeDescription: true) restores correct whole-subject enforcement.
Extended reasoning...
The bug: .github/workflows/check-commit-message.yml configures gsactions/commit-message-checker@v2 with pattern: '^(Bump|Merge|Revert|Reapply|build\(deps(-dev)?\): bump|([A-Z]+-[0-9]+))\s+.+' and flags: "gm". That action builds new RegExp(pattern, flags) and runs .test() against the full commit message (subject + body concatenated), since excludeDescription defaults to false.
Why the "m" flag breaks the intent: In JavaScript regex semantics, the m (multiline) flag changes ^ and $ to match the start/end of any line within the string, not just the start/end of the whole string. Combined with .test(), which returns true if the pattern matches anywhere in the string, this means the checker no longer verifies that the commit message begins with a JIRA key — it only verifies that some line, anywhere in the message, starts with one of the allowed prefixes.
Concrete proof: Consider a commit with subject update stuff and body EA-2130 follow-up. The full message is:
update stuff
EA-2130 follow-up
With flags gm, new RegExp(pattern, 'gm').test(message) returns true, because ^EA-2130 follow-up matches the pattern starting at the third line. Without the m flag, the same regex only matches at position 0 of the string, so it correctly returns false for this message (since the subject update stuff doesn't start with a JIRA key or an allowed exception like Bump/Merge/Revert/Reapply/build(deps)). This was independently confirmed by running the two RegExp variants against sample messages in Node — a conforming subject like EA-2130 do the thing still passes without m, while the non-conforming example above only passes because of m.
Realistic trigger scenarios: squash-merge commit messages that concatenate multiple original commit subjects into the body, multi-line commit bodies that reference a ticket on a later line, or a body line that happens to start with "Merge"/"Bump"/etc. All of these can cause the checker to pass a commit whose actual subject line violates the CLAUDE.md/AGENTS.md rule: "Every commit message must begin with a JIRA issue key."
Why nothing else in the repo catches this: This is a brand-new workflow file introduced by this PR — there's no other commit-subject validation in the pipeline, so this regex is the sole enforcement mechanism for the "begins with JIRA key" policy. The action itself doesn't warn about the 'm+ anchor' interaction, since the flags are fully caller-controlled.
Fix: Drop the m flag (use flags: "g" or omit flags), which restores ^/$ to whole-string anchors so the pattern only matches when the entire message begins with one of the allowed prefixes. Alternatively, set excludeDescription: true so only the first line (subject) is checked, which also sidesteps the multi-line matching issue entirely.
Severity: All verifiers converged on nit — this weakens a CI/commit-hygiene guardrail rather than causing a runtime crash, data loss, or security gap. It only lets through a narrow class of non-conforming commit messages (those with an unusual, ticket-token-prefixed body line), conforming commits are unaffected, and it's trivially fixed with a one-line change to the workflow config.
| // Collect ALL registered parents (not just the one we follow) | ||
| const candidates = usedBy.filter( | ||
| (p) => p.component && p.component !== currentName, | ||
| ); | ||
| for (const c of candidates) { | ||
| if ( | ||
| registry[c.component] && | ||
| c !== candidates.find((p) => registry[p.component]) | ||
| ) { | ||
| alternateRegistered.push({ | ||
| component: c.component, | ||
| registryKeys: registry[c.component].map((r) => r.key), | ||
| reachedFrom: currentName, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Pick next parent: prefer registered, then screens, then first | ||
| const registeredParent = candidates.find((p) => registry[p.component]); | ||
| const screenParent = candidates.find( | ||
| (p) => p.file.includes('/screens/') || p.file.includes('/pages/'), | ||
| ); | ||
| const nextParent = registeredParent || screenParent || candidates[0]; |
There was a problem hiding this comment.
🟡 In traceComponent(), the alternateRegistered exclusion check (c !== candidates.find((p) => registry[p.component])) compares by array-element identity rather than by component name, so it only excludes the first candidate object matching a registered component. If a second distinct usedBy entry resolves to the same registered component/registryKey (e.g. via the basename(dirname(...)) fallback when getPrimaryExport returns null), it slips past the identity check and gets pushed into alternateRegistered" even though it is the same route registeredParent(same line) picks for the main resolved chain — somain()prints that route twice, once as the resolved route and again under "Alternate paths". Fix by excluding/deduping onc.component(or the resultingregistryKeys`) instead of object identity.
Extended reasoning...
What the bug is: In resolve_route.mjs's traceComponent() (lines ~501-523), when building the list of alternate registered parents, the code excludes the 'primary' registered candidate with a reference-identity check:
if (
registry[c.component] &&
c !== candidates.find((p) => registry[p.component])
) {
alternateRegistered.push({ ... });
}candidates.find(...) deterministically returns the first array element whose component is registered. The comparison c !== <that first element> is an object-identity check. If a second, distinct entry in candidates happens to have the same component name as the first (i.e. same registry key) but is a different array element (different object reference, e.g. arising from two different referencing files), that second entry is not identity-equal to the one .find() returned, so it passes the check and gets pushed into alternateRegistered — even though it points at the exact same registered component/route.
How it manifests / code path: A few lines later, registeredParent = candidates.find((p) => registry[p.component]) (line ~519) runs the same .find() call and selects that identical first entry as nextParent, continuing the main resolution chain. That entry ultimately becomes result.resolved (via the chain build further down). So the same component/registryKey ends up in both result.resolved and result.alternatePaths.
Why existing code doesn't prevent it: There is no dedup step on alternateRegistered by component or registryKeys anywhere else in the function, and main()'s output block (which prints 'Route (key)' and then iterates 'Alternate paths') does not cross-check the alternate list against the resolved route before printing. Nothing catches the duplicate before it reaches stdout.
Step-by-step proof:
- Suppose while tracing,
usedBy/candidatescontains two array entries, A and B, both withcomponent: 'Foo'(this can legitimately happen — e.g. two different referencing files whoseprimaryExportis null, both falling back tobasename(dirname(refPath)), and both directories happen to produce the same folder-derived name — or duplicate reference entries for the same effective component from ts-morph'sfindReferences()). registry['Foo']exists (it's registered inmainRegistry.tsx).candidates.find((p) => registry[p.component])returns A (the first match in iteration order).- In the exclusion loop: for A,
A !== Ais false → correctly skipped. For B,B !== Ais true (different object) →registry['Foo']is truthy → B is pushed intoalternateRegisteredwithregistryKeys: registry['Foo'].map(...), identical to what A would produce. - After the loop,
registeredParent = candidates.find(...)again returns A, and A becomesnextParent/continues the chain, eventually landing inresult.resolved.registryKey. main()prints the resolved route once fromresult.resolved, then iteratesresult.alternatePaths, printing the same registryKey/route again under 'Alternate paths' — a false duplicate that looks like a second, genuinely different way to reach the target component.
Impact: This is a diagnostic/dev-tool script (part of a new Claude Code skill, resolve-route) used by a human or agent to find where a component is reachable from in the codebase. The impact is a misleading duplicate line in the printed report — it does not affect the correctness of the resolved route itself, cause a crash, or affect any production code path. All three independent verifiers confirmed the logic defect but converged on nit severity for exactly this reason: the trigger condition (two distinct candidate entries resolving to the same registered component name) is a narrow edge case, and even when triggered the only consequence is cosmetic — a duplicated report line a human reviewer would need to notice and could reasonably ignore.
Fix: Change the exclusion check to compare by c.component (or by the resulting registryKeys) instead of object identity, e.g. c.component !== registeredParent?.component computed once, or dedupe alternateRegistered by component/registryKeys before returning it from traceComponent().
| --- | ||
| name: design-doc-trigger | ||
| description: Triggers design doc scaffolding when changes require architecture review | ||
| triggers: | ||
| - New dependency not in approved-tech.yaml | ||
| - New service creation (Dockerfile, deployment manifests) | ||
| - New HTTP client to internal service | ||
| - GraphQL schema changes adding types/mutations | ||
| - New FHIR resource usage | ||
| - New Kafka event/topic | ||
| --- | ||
|
|
||
| # Design Doc Trigger | ||
|
|
||
| You detect when changes require architecture review and scaffold the appropriate document. Per b.well's process, not all changes need formal design docs - but when they do, missing one causes PR delays. | ||
|
|
||
| ## Tone | ||
|
|
||
| Helpful but firm. You're preventing process friction by scaffolding docs **before** the user hits PR review. | ||
|
|
||
| ## Detection Rules | ||
|
|
||
| ### Rule 1: New Dependency | ||
|
|
||
| **Trigger:** User adds dependency not in `approved-tech.yaml` | ||
|
|
||
| **Detection:** | ||
| - New package in `package.json`, `requirements.txt`, `pom.xml`, `build.gradle`, `go.mod` | ||
| - Dependency name not found in approved-tech.yaml |
There was a problem hiding this comment.
🟡 The new design-doc-trigger/SKILL.md cites specific AGENTS.md line numbers for its detection rules, but those line numbers don't match the actual content in the AGENTS.md added by this same PR (e.g. Rule 2 and Rule 5 both point to line 242, which is a blank separator line, not the Design Doc or FDR requirement text). This will send an agent/reader to the wrong part of the document; consider citing section headers/anchors instead of line numbers (the org baseline itself recommends this for exactly this reason).
Extended reasoning...
What's wrong: design-doc-trigger/SKILL.md hardcodes several AGENTS.md line N citations inside its detection-rule prose, but every one of them is stale/incorrect relative to the AGENTS.md that this same PR adds:
- Rule 2 (New Service) says 'This requires a Design Doc per b.well review process (AGENTS.md line 242)'. Line 242 of the new
AGENTS.mdis a blank separator line immediately before the## Architecture Decision Recordsheader — it contains no Design Doc requirement at all. - Rule 5 (New FHIR Resource) cites the same line 242 for the FDR process. The actual FDR-process prose ('FHIR data modeling decisions go through the FDR...') lives at line 66, and the Process References FDR bullet is around line 263.
- Rule 3 (cross-service dependency) cites line 267, which is the generic closing sentence of the 'Is it NEW?' test in Process References — not anything specifically about cross-service dependencies.
- Rule 1's 'line 264' (new technologies require an ADR) also lands on the Public API/Tech Design Review bullet rather than the ADR/new-technology text.
Why this happened: SKILL.md and AGENTS.md are both new files introduced by this PR, so there was no prior version to diff against — the line numbers were presumably written against a draft of AGENTS.md that didn't survive to the final content, or were guessed/approximated rather than verified against the shipped file.
Why nothing catches this automatically: these are prose citations inside markdown instructional content consumed by an AI agent (or a human) reading the skill — there's no linter or test that checks a line-number reference against the target file's actual content, so drift like this is invisible until someone (or an agent) actually follows the citation.
Impact: When Claude (or a developer) is executing this skill and needs to double-check the underlying policy, following 'AGENTS.md line 242' lands on a blank line next to an unrelated ADR section header. The agent would need to re-search AGENTS.md for the real content anyway, so the citation actively misleads rather than saves time. Nothing breaks functionally — the skill's prose descriptions of the policies (e.g. 'new services need a Design Doc', 'new FHIR resources should go through FDR') are still correct in substance, only the pinpoint citations are wrong.
Proof (step-by-step):
- Open the
AGENTS.mdadded in this PR and jump to line 242. It reads simply---or is blank, sitting directly above line 243's## Architecture Decision Records. - Search the file for 'FDR' — the real explanation ('FHIR data modeling decisions go through the FDR (FHIR Design Review) process...') is at line 66, roughly 176 lines away from the cited line 242.
- Jump to line 267 (cited by Rule 3) — it reads as the closing 'Is it NEW?' sentence of the Process References section, generic guidance unrelated specifically to cross-service dependencies.
- Jump to line 264 (cited by Rule 1) — it's the Public API/Tech Design Review bullet, not the ADR/new-technology-requires-ADR text.
Suggested fix: Replace the hardcoded line numbers with references to section headers or anchors, e.g. 'see the FDR process in AGENTS.md's Process References section' or an anchor-style citation (AGENTS.md#process-references) as the org baseline's own docs/knowledge-substrate.md convention recommends — precisely to avoid this kind of drift when the target document is edited.
| # icanbwell - Copilot Instructions | ||
|
|
||
| You are working in a cloud-native, multi-tenant, HIPAA-compliant healthcare platform. The platform is FHIR-native, event-driven (Kafka + CloudEvents), and exposes capabilities through a federated GraphQL gateway. | ||
|
|
||
| ## Hard Constraints | ||
| - Tenant isolation is mandatory on every data access path. Not optional. | ||
| - No PHI/PII in logs, test fixtures, example data, comments, or PR descriptions. | ||
| - No new technology, vendors, or patterns without checking approved-tech.yaml and EA review. | ||
| - Public API changes require a Tech Design Review. | ||
| - Event-driven first. Default to async via Kafka. Justify sync. | ||
| - Client-facing access goes through the federated GraphQL gateway. No bypass. | ||
|
|
||
| ## Design Defaults | ||
| - Program to interfaces, not implementations. Vendor integrations behind capability abstractions. | ||
| - Composition over inheritance. Strategy pattern over growing conditionals. | ||
| - Dependency injection at boundaries. No hidden global state. | ||
| - Idempotent consumers. Assume at-least-once delivery. | ||
| - Parameterized tests for functions with more than two input variations. | ||
| - Mock only at external boundaries. | ||
|
|
||
| ## Before Coding | ||
| - Find and use the repo's canonical build/test/lint commands. Do not guess. | ||
| - Propose a plan for non-trivial changes. Call out tenancy, PHI, contract, and dependency risks. | ||
| - Check approved-tech.yaml before introducing any dependency. | ||
| - If your change touches public API, events, or cross-service behavior, reference the governing artifact (TDD, FDR, ADR, AsyncAPI). | ||
|
|
||
| ## Repo-Specific Instructions | ||
| Check .github/copilot-instructions.md in the specific repository for repo-level context, commands, and additional guidelines that extend these org-wide instructions. |
There was a problem hiding this comment.
🟡 The org-wide copilot-instructions.md is placed at the repo root, but GitHub Copilot only auto-discovers repository custom instructions at .github/copilot-instructions.md, so this file is never actually loaded by Copilot. The file even instructs readers to check .github/copilot-instructions.md for repo-level context, confirming that's the intended convention — move (or symlink) the file to .github/copilot-instructions.md.
Extended reasoning...
The bug: copilot-instructions.md is added at the repository root in this PR, but GitHub Copilot's custom-instructions feature only auto-discovers a file at .github/copilot-instructions.md. A root-level file of the same name is simply invisible to Copilot — it is never read or applied. Since the PR description explicitly calls this file the 'Lightweight version for GitHub Copilot,' its entire purpose is defeated by its placement.
Where this shows up in the diff: The new file lives at copilot-instructions.md (repo root), confirmed by the change list (copilot-instructions.md, no corresponding .github/copilot-instructions.md). Nothing else in the PR creates or symlinks a copy under .github/.
The self-inconsistency: Line 28 of the file itself tells readers to 'Check .github/copilot-instructions.md in the specific repository for repo-level context' — i.e., the author already knows .github/copilot-instructions.md is the standard location for Copilot instructions, yet placed the org-wide file itself at the repo root instead of there. CODEOWNERS also only lists /AGENTS.md and /CLAUDE.md for EA ownership, omitting copilot-instructions.md entirely, which further suggests the root placement wasn't a deliberate, reviewed decision.
Why nothing catches this today: This is a plain documentation/config file with no build step, linter, or test that validates its location or its consumption by Copilot. Nothing else in the repo currently depends on Copilot actually loading these instructions, so the mismatch is silent — Copilot simply won't have the org-wide architecture guidance available to it in this repo.
Proof, step by step:
- PR adds
copilot-instructions.mdat the repo root (per the changed-files list). - GitHub's documented behavior: repository-wide custom instructions for Copilot are only auto-loaded from
.github/copilot-instructions.md. - There is no file at
.github/copilot-instructions.mdin this repo (confirmed absent from the changeset and from CODEOWNERS' explicit list of instruction files). - Therefore, Copilot in this repository will never load the guidance in
copilot-instructions.md— it is dead documentation from Copilot's perspective, even though a human reading the repo would find it. - Confirmation that the author intended the
.github/convention: the file's own body (line 28) references.github/copilot-instructions.mdas where repo-level context should live.
Fix: Move the file to .github/copilot-instructions.md, or keep it at the root and add a symlink at .github/copilot-instructions.md pointing to it — mirroring the pattern already used for CLAUDE.md → AGENTS.md in this same PR. Either approach makes Copilot actually pick up the org-wide baseline.
Severity rationale: This is a documentation-only, purely additive change in an example/instruction-sync repo. Nothing breaks at runtime, no build or test fails, and there's no regression — the file simply doesn't yet do what it's meant to do for Copilot specifically. That makes this a nit: worth a quick fix, but not blocking merge.
|
Closing — opened by an aborted org-wide instruction-sync run. The sync's content-clobber gap is fixed and approved in icanbwell/.github#28, and the corrected sync will also ship the knowledge-substrate dirs the skills reference. A clean sync re-runs after #28 merges; no action needed. Branch deleted. |
2 participants
Architecture Instruction File Sync
This PR updates the organization-wide AI agent instruction baseline files from
icanbwell/.github.Changes
Source commit: 62f8288431418749382e0b673b363841d8f8977d
Review Notes
Review and merge when ready. These are org-wide architecture instruction updates from EA team. These files are purely additive and don't modify existing code.
Related: EA-2130