EA-2136 sync architecture instruction files from org baseline - #195
EA-2136 sync architecture instruction files from org baseline#195bwell-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: fcde6dde7e728334359e707bc2b37853dbd94e51
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # 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.
🔴 GitHub Copilot only auto-loads repository custom instructions from .github/copilot-instructions.md, but this PR adds the file at the repo root (copilot-instructions.md) with no copy in .github/. As shipped, Copilot never reads this content, so none of the org-wide Hard Constraints (tenant isolation, no PHI in logs/fixtures, approved-tech.yaml check, TDR for public API changes, event-driven-first, gateway-only access) reach Copilot users in this repo — defeating the PR's stated goal. The file's own last line also points to .github/copilot-instructions.md for repo-level context, which doesn't exist, so that reference is dead too. Fix: move the file to .github/copilot-instructions.md.
Extended reasoning...
The bug: GitHub Copilot's repository custom-instructions feature has one documented auto-load path: .github/copilot-instructions.md. A file simply named copilot-instructions.md sitting at the repository root is not recognized by Copilot Chat or the Copilot coding agent — it is just an inert markdown file as far as Copilot is concerned. This PR adds exactly that: copilot-instructions.md at the repo root, described in the PR body as 'Lightweight version for GitHub Copilot,' with no corresponding file under .github/.
Verification: I checked the repository directly. copilot-instructions.md exists only at the root (confirmed via find), and .github/ contains only CODEOWNERS and workflows/ — no copilot-instructions.md. I also read the file's final section, titled 'Repo-Specific Instructions,' which reads: 'Check .github/copilot-instructions.md in the specific repository for repo-level context, commands, and additional guidelines that extend these org-wide instructions.' That line points at a path that does not exist anywhere in this repo, so it is a dead self-reference — additional evidence that .github/ is the intended home for this content, not the root.
Why nothing else catches this: The PR is purely additive (new files, no code changes) and explicitly framed as 'these files are purely additive and don't modify existing code,' so there is no CI check, test, or lint rule in this repository that validates where Copilot-specific instruction files must live. Nothing fails to build or test red — the content is simply never consumed by its intended tool.
Step-by-step proof:
- Open the repo root:
copilot-instructions.mdis present (verified: 28 lines, ~1.7KB). - Open
.github/: contains onlyCODEOWNERSandworkflows/(verified via directory listing) — nocopilot-instructions.md. - Per GitHub's documented behavior, Copilot Chat and the Copilot coding agent load repository-wide custom instructions exclusively from
.github/copilot-instructions.md. Since that path is empty, Copilot in this repo effectively has zero custom instructions from this PR. - Contrast with the AGENTS.md/CLAUDE.md files added by the same PR, which sit at the repo root by design because that is the convention their respective tools (Claude Code, and the AGENTS.md ecosystem) actually read from — so root-level placement is correct for those two files, but wrong specifically for the Copilot file, which has a different, GitHub-enforced required path.
- The fix is a one-line move: relocate the content to
.github/copilot-instructions.md. This also resolves the dead self-reference in the file's last line, since that path would then exist.
Impact: This is a functional defect in the PR's deliverable — the Copilot-specific instructions (tenant isolation, no PHI in logs/fixtures, approved-tech.yaml check, TDR requirement, event-driven-first, gateway-only access) are completely unreachable by Copilot as shipped, even though the PR's whole purpose for this file was to deliver a 'lightweight version for GitHub Copilot.' There's no runtime/security blast radius since it's a static instructions file, but the feature is 100% non-functional for its only consumer until moved.
| # AGENTS.md — HP Validation Tests | ||
|
|
||
| This document describes the conventions, patterns, and execution instructions for authoring and running the Karate-based end-to-end validation tests in this project. | ||
|
|
||
| --- | ||
|
|
||
| ## Table of Contents | ||
|
|
||
| 1. [Project Overview](#project-overview) | ||
| 2. [Repository Structure](#repository-structure) |
There was a problem hiding this comment.
🔴 AGENTS.md (and therefore CLAUDE.md, which is a new symlink to it) was synced with the wrong source content: it contains the repo-specific 718-line 'HP Validation Tests' Karate documentation instead of the org-wide 401-line architecture baseline (tenant isolation, PHI/PII rules, approved-tech.yaml, event-driven-first, etc.) that the PR description says it delivers. This is corroborated by copilot-instructions.md in the same PR, which correctly received the real generic baseline as its 'lightweight version' — confirming AGENTS.md got the wrong file from the org sync. As written, engineers/agents relying on CLAUDE.md/AGENTS.md for architecture guidance get Karate test-authoring instructions instead.
Extended reasoning...
What's wrong: This PR (EA-2136) is described as syncing architecture instruction files from icanbwell/.github, with AGENTS.md said to be the 'Organization-wide baseline (401 lines) codifying b.well architecture standards,' and CLAUDE.md added as a new symlink to it 'for Claude Code compatibility.' In the actual diff, AGENTS.md is a new 719-line file whose first line is # AGENTS.md — HP Validation Tests, and its entire content is this repository's own Karate/CQL end-to-end test documentation — karate-config.js conventions, util/*.feature reference, CQL evaluation patterns, FHIR payload file listings, and Gradle test-execution commands. None of this relates to org-wide architecture standards (tenant isolation, PHI/PII handling, approved technology, event-driven design, etc.).\n\nCode path that triggers it: Any engineer or AI agent working in this repo and consulting CLAUDE.md for architecture guidance follows the symlink (CLAUDE.md -> AGENTS.md, mode 120000, added by this same commit) straight into the Karate test doc. There is no fallback — this is the only content CLAUDE.md now serves.\n\nWhy nothing in this PR catches it: The PR is self-described as 'purely additive,' automated/scripted from a source commit in icanbwell/.github, and the review notes just say 'these are org-wide architecture instruction updates... review and merge when ready.' There's no content-diffing or drift check between the intended baseline and what actually landed in the file, so a copy/sync mistake (wrong source file selected for AGENTS.md) passes through unnoticed.\n\nStep-by-step proof:\n1. Open AGENTS.md in this PR's diff — line 1 reads # AGENTS.md — HP Validation Tests, and the table of contents that follows covers 'Project Overview,' 'Repository Structure,' 'Configuration (karate-config.js),' 'Authoring CQL Evaluation Tests,' 'Payload Files,' and 'Executing Tests' — all Karate/CQL test-authoring topics, 718 lines total (not the '401 lines' the PR description claims for the architecture baseline).\n2. Open copilot-instructions.md in the same diff — it contains 'multi-tenant, HIPAA-compliant... Tenant isolation is mandatory... No PHI/PII in logs... check approved-tech.yaml... Event-driven first... federated GraphQL gateway,' and frames itself as the 'Lightweight version' of a fuller baseline.\n3. Since the PR description states copilot-instructions.md is the lightweight version and AGENTS.md is the full version of the same org baseline, they should overlap substantially in subject matter (tenancy/PHI/tech-approval/events), just at different depths. They don't overlap at all — one is architecture policy, the other is Karate test authoring instructions for entirely unrelated (pre-existing) local test suite documentation.\n4. Confirm CLAUDE.md is a new symlink to AGENTS.md (mode 120000) added in this same commit — so this isn't a pre-existing, already-tolerated situation; the PR is the one wiring CLAUDE.md to point at this content for the first time.\n5. Conclusion: the sync process that populated these files pulled the wrong source content into AGENTS.md (likely copying this repo's pre-existing local Karate documentation instead of fetching the org baseline from icanbwell/.github), and copilot-instructions.md shows what the correct content should have looked like.\n\nImpact: No runtime breakage — this is instruction/documentation content only. But it silently defeats the PR's entire stated purpose (EA-2136: give Claude Code and other agents/engineers the org architecture baseline via CLAUDE.md). Anyone trusting CLAUDE.md/AGENTS.md for tenancy, PHI/PII, approved-tech, or event-driven-design guidance in this repo gets nothing relevant — they get Karate test-writing instructions instead, with no indication anything is wrong.\n\nFix: Replace AGENTS.md's content with the actual org-wide baseline from icanbwell/.github (source commit fcde6dde... per the PR description) — the same content the 401-line count and copilot-instructions.md's framing imply. If the existing Karate/CQL testing documentation is still valuable for this repo, it should be preserved separately (e.g., restored under its own file, such as a README or a dedicated docs/karate-tests.md) rather than overwriting the org baseline destination.
| name: PR Architecture Review | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, synchronize, reopened] | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
|
|
||
| jobs: | ||
| call-review: | ||
| uses: icanbwell/.github/.github/workflows/pr-architecture-review-reusable.yml@main | ||
| with: | ||
| pr_number: ${{ github.event.pull_request.number }} | ||
| pr_author: ${{ github.event.pull_request.user.login }} | ||
| base_ref: ${{ github.event.pull_request.base.ref }} | ||
| head_ref: ${{ github.event.pull_request.head.ref }} | ||
| head_sha: ${{ github.event.pull_request.head.sha }} | ||
| secrets: | ||
| gh_token: ${{ secrets.GITHUB_TOKEN }} | ||
| bwell_dev_pat: ${{ secrets.BWELL_DEV_PAT }} |
There was a problem hiding this comment.
🔴 This new workflow (.github/workflows/pr-architecture-review.yml) triggers on pull_request_target — running with base-repo privileges and full secrets even for fork PRs — while forwarding the untrusted head_ref/head_sha plus secrets.BWELL_DEV_PAT (a personal access token) and GITHUB_TOKEN to a reusable workflow, with pull-requests: write permission. This is the canonical GitHub Actions 'pwn request' pattern: if the reusable workflow checks out or executes anything from the fork's head commit while holding the PAT, a malicious fork PR could exfiltrate the token or write back to the repo. Recommend splitting into a pull_request job (fork-restricted token, no secrets, does the analysis) and a separate pull_request_target job that only posts the comment without checking out head content, dropping the PAT if possible, and pinning the reusable workflow to a SHA instead of @main.
Extended reasoning...
What the bug is. .github/workflows/pr-architecture-review.yml is a newly-added workflow in this PR. It triggers on pull_request_target (line 4), which — per GitHub's own documented security guidance — always runs with the base repository's permissions and full secrets access, even when the triggering PR comes from a fork. The workflow grants pull-requests: write, extracts head_ref/head_sha from the PR event (lines 18-19), and forwards them — along with secrets.BWELL_DEV_PAT (a personal access token, not just the default GITHUB_TOKEN) and GITHUB_TOKEN itself (lines 20-22) — into the reusable workflow icanbwell/.github/.github/workflows/pr-architecture-review-reusable.yml@main.\n\nThe code path that triggers it. This is the textbook 'pwn request' anti-pattern that GitHub's security docs and tools like zizmor/CodeQL explicitly flag: (1) a privileged trigger (pull_request_target) that (2) receives an untrusted ref/commit from a fork (head_ref/head_sha) while (3) holding secrets with write capability. The presence of head_sha being passed downstream strongly implies the reusable workflow needs to check out that commit to actually review it — an architecture-review bot has to read the PR's changed files somehow. If that checkout is followed by any execution against fork content (a lint step with a local config file, a script invocation, a dependency install with a postinstall hook, etc.) while the job still holds BWELL_DEV_PAT, a malicious fork PR can exfiltrate that PAT or use it to write to any repo it can reach.\n\nWhy existing code doesn't prevent it. Nothing in this calling workflow separates the 'read untrusted content' concern from the 'hold privileged secrets' concern — both happen in the same job, under the same trigger. The reusable workflow lives in icanbwell/.github and isn't part of this diff, so its internal safeguards (e.g., whether it actually avoids executing fork content) can't be verified here — but that cuts both ways: the risky inputs (privileged trigger + untrusted ref + PAT) are fully present and confirmed in this file regardless of what the reusable workflow does with them. The safe burden should be on proving the reusable workflow never executes fork-controlled content while holding the PAT, not on assuming it's fine.\n\nConcrete proof-of-concept walkthrough. (1) An attacker forks the repo and opens a PR that includes a file the review bot is likely to read/lint/parse (e.g., a modified AGENTS.md, a skill script, or a config file referenced by tooling in the reusable workflow). (2) The PR triggers pull_request_target on the base repo, which runs with full secrets and pull-requests: write. (3) The calling workflow passes head_sha to the reusable workflow, which (per its likely purpose of reviewing PR content) checks out that commit to read the diff. (4) If any step in that reusable workflow evaluates or executes something derived from the checked-out fork content (rather than purely fetching the diff via the GitHub API), the attacker's payload runs with access to BWELL_DEV_PAT and GITHUB_TOKEN, letting them print/exfiltrate the PAT via a workflow log or an outbound HTTP call, or use it directly to push/comment/modify other repos the PAT can reach.\n\nOn the refutation. One reviewer argued this is 'speculative' because the exploit depends on unverified reusable-workflow behavior, and that forwarding head_sha + a PAT to a same-org reusable workflow for read-only analysis is 'standard, intended design.' That's true as far as it goes — but it describes the best case, not a guarantee, and the whole point of the pwn-request pattern is that the calling workflow is exactly where the mitigation belongs, because the reusable workflow's future behavior (or a supply-chain compromise of @main, which is unpinned) is outside this repo's control. The fact that the finding can't prove exploitation from this diff alone doesn't make the configuration safe — it makes it unverified, which for a privileged secret-holding trigger should default to 'fix defensively' rather than 'assume it's fine.' The refutation's point about this being an EA-owned baseline file synced from icanbwell/.github doesn't make it non-actionable either: the fix (splitting the checkout/analysis job under pull_request from a checkout-free comment-posting job under pull_request_target, and/or dropping the PAT, and/or pinning the reusable workflow to a SHA) can be applied entirely within this file, independent of the reusable workflow's internals.\n\nHow to fix. Split the workflow into two jobs: (a) a pull_request-triggered job that checks out the PR head and performs the actual analysis — this runs with the fork's restricted, read-only token and no secrets, so even if it executes fork content there's nothing sensitive to steal; and (b) a separate pull_request_target-triggered job that receives only the analysis output (e.g., via artifact or workflow_run) and posts the PR comment using GITHUB_TOKEN/the PAT, without ever checking out the PR head. Additionally, avoid forwarding BWELL_DEV_PAT if GITHUB_TOKEN with pull-requests: write is sufficient, and pin icanbwell/.github/.github/workflows/pr-architecture-review-reusable.yml to a commit SHA rather than @main to close the supply-chain angle.
|
|
||
| ``` | ||
| You're working with FHIR `MedicationRequest` for prescription ordering. | ||
|
|
||
| Let me check if there's an existing FDR for this... | ||
|
|
||
| [Search Confluence via Atlassian MCP: | ||
| cloudId: dc9c52d9-3f1d-4037-9c66-a57f1079b857 | ||
| cql: "type=page AND space=ENTARCH AND title~'FDR' AND title~'MedicationRequest'" | ||
| ] | ||
| ``` | ||
|
|
||
| **If FDR exists:** | ||
| "Found FDR: 'FDR-MedicationRequest-Prescription-Ordering' (https://...) | ||
|
|
||
| **Follow this FDR:** |
There was a problem hiding this comment.
🟡 Line 42 of .claude/skills/fhir-resource-design/SKILL.md hardcodes a literal Atlassian cloudId (dc9c52d9-3f1d-4037-9c66-a57f1079b857) in an example MCP call, whereas four sibling skills added in this same PR (fhir-design, fhir-design-review, tech-design, tech-design-review) explicitly instruct discovering the cloudId at runtime via getAccessibleAtlassianResources and state 'never hardcode it,' since cloudIds are per-instance and can be wrong for a different tenant or after a migration. Recommend aligning this skill with the same runtime-discovery pattern for consistency.
Extended reasoning...
The bug: .claude/skills/fhir-resource-design/SKILL.md:42 contains a hardcoded Atlassian cloudId inside an example MCP-call code block:
cloudId: dc9c52d9-3f1d-4037-9c66-a57f1079b857
This is the only place in the entire .claude/skills/ tree where this literal cloudId appears baked into instructional content. Every other skill added in this same PR that talks to Atlassian establishes the opposite convention, in nearly identical language:
fhir-design/SKILL.md:20fhir-design-review/SKILL.md:18tech-design/SKILL.md:37tech-design-review/SKILL.md:16
Each of these instructs the agent to discover the cloudId at runtime via getAccessibleAtlassianResources (or the equivalent MCP call) and explicitly says 'never hardcode it.' The stated rationale, repeated across those files, is that cloudIds are per-Atlassian-instance identifiers — they can legitimately differ across tenants, and a cloudId that is correct today can become stale after an Atlassian-side migration.
Why this matters: fhir-resource-design/SKILL.md is a Claude Code skill file — its content is instructional, and an agent following it verbatim could copy the hardcoded cloudId into a live MCP call instead of resolving it dynamically, silently querying the wrong (or a nonexistent) Atlassian cloud instance for a tenant whose actual cloudId differs. That failure mode is exactly what the sibling skills' 'never hardcode it' rule exists to prevent, and this file is the one place in the PR that doesn't follow it.
Concrete walkthrough: Imagine an org other than icanbwell forks this skill set (or icanbwell's own Atlassian instance migrates to a new cloudId, which the sibling-skill docs explicitly call out as a real scenario). An agent invoking fhir-resource-design would copy cloudId: dc9c52d9-3f1d-4037-9c66-a57f1079b857 straight into a Confluence/Jira MCP call. If a sibling skill's flow had instead been followed, the agent would call getAccessibleAtlassianResources first and use whatever cloudId comes back — correct regardless of tenant or migration state. The hardcoded version breaks exactly in the scenario the other four skills were written to guard against.
Why existing code/review doesn't catch it: This is a documentation/instruction-consistency issue inside net-new skill files added by this same PR, so there's no existing lint or test that flags a literal UUID appearing in a markdown code fence — the inconsistency is only visible by cross-referencing the five sibling skills against each other, which is what the multiple independent verifiers here did.
Severity: All three independent verifiers who examined this concluded nit, and I agree. The value only appears inside an illustrative example block (not a hard runtime instruction that will definitely execute), and it is plausibly the actual, currently-correct icanbwell Atlassian cloudId — design-doc-trigger/SKILL.md in this same PR independently references 'the current icanbwell Atlassian cloudId,' suggesting this concrete value is not simply wrong for this org today. So merging as-is causes no immediate failure. It's a genuine, PR-introduced inconsistency worth fixing for consistency and future-proofing (per the sibling skills' own stated rationale), but not something that blocks merge.
Suggested fix: Replace the hardcoded cloudId line in the example with the same runtime-discovery instruction used by the sibling skills, e.g. call getAccessibleAtlassianResources first and use the returned cloudId, matching the pattern in fhir-design/SKILL.md:20.
|
Closing — this sync run started before icanbwell/.github#34 removed skill-syncing entirely, and completed with runner-workspace contamination: the .claude/skills content here mixes real skills with directories from unrelated repos that never belonged in the org baseline. See icanbwell/.github#38 (excludes .github from its own sync targets) and #34 (skills no longer sync at all). A clean sync will follow. |
2 participants
Architecture Instruction File Sync
This PR updates the organization-wide AI agent instruction baseline files from
icanbwell/.github.Changes
Source commit: fcde6dde7e728334359e707bc2b37853dbd94e51
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