Add flightcheck developer guide (AGENTS.md)

[Graham McMynn (GrahamMcMynn)]

Summary

Adds a developer guide scoped to the flightcheck tool at \scripts/flightcheck/AGENTS.md. This documents the knowledge needed to efficiently add new checks without trial-and-error against live APIs.

Motivation

While implementing CONFIG-013 (PR #37), significant time was spent:

• Trying the wrong API (Dataverse \�otcomponents.data\ is YAML config, not status)
• Discovering the Island Gateway API exists and is the correct source for runtime state
• Finding the required \x-cci-*\ headers via an external repo
• Learning that BAP env ID Dataverse env ID

This guide captures those learnings so the next check can be implemented correctly on the first attempt.

Contents

• Architecture Overview table mapping API layers to clients, auth scopes, and available data
• Island Gateway API Reference — endpoint, headers, env ID discovery, response structure
• Dataverse \�otcomponents\ Entity key gotchas (YAML not JSON, no \msdyn_\ prefix, statecode crawl status)
• Design Principles no misleading results, fail loudly, no fabricated URLs
• Adding a New Check step-by-step checklist including guidance for when a new API is needed

[John Nguyen (johnguy0)]

Doc is genuinely useful — it captures the gotchas you spent CONFIG-013 hours on, and that's exactly the kind of thing that pays for itself the next time someone adds a check. Two structural things worth tightening before merge:

1. The "Dataverse = configured, Gateway = working" framing in the Architecture Overview doesn't match what the kit actually does today. Configured state in this kit comes from local YAML files (scripts/flightcheck/checks/local_files.py), not from Dataverse. If a future check author follows this guide literally, we'll get a duplicate Dataverse query for something already validated locally — two sources of truth that can disagree.

2. The "attached to runner" pattern is documented but the shape of runner isn't declared anywhere. New contributors have to read cli.py to know what's on it. Either enumerate the attributes here (runner.config, runner.env_url, runner.dv_token, runner.env_id, runner.graph, runner.pp_admin, runner.pva) or — better — declare them as typed Optional fields on FlightCheckRunner in runner.py so they're discoverable from the type.

The "No misleading results" principle is the most valuable thing in this doc. Don't lose it in the cleanup. Inline nits on file paths.

[John Nguyen (johnguy0)]

Nothing security-relevant in this PR. Worth noting for the record:

• The shared MSAL token cache at my/.token_cache.bin is referenced as a contributor pattern. Already covered by my/** in .gitignore, so no leak risk.
• 96ff4394-9197-43aa-b393-6a41652e21f8 is the public Power Virtual Agents 1P app ID, not a secret.
• No untrusted input flowing anywhere. No new dependencies.

Deferring to other personas.

[John Nguyen (johnguy0)]

Doc-only PR, no perf angle. The closest thing is the documented "Read all bot components" call which has no pagination, but that's a faithful description of how the Island Gateway endpoint actually behaves — not something this doc introduced. Deferring.

[John Nguyen (johnguy0)]

Solid reference material — exactly the kind of doc that pays for itself the next time. Two things worth tightening so a contributor can actually act on the steps:

1. Status taxonomy is under-specified. Principle 1 only mentions SKIPPED vs PASSED, but the runner exposes five statuses (SKIPPED, NOT_CONFIGURED, WARNING, FAILED, ERROR) and existing checks split between them in ways this doc doesn't capture. Inline comment with the decision tree.

2. "Adding a New Check" assumes you know the CheckResult shape. Steps 3–5 say "add a check function" without showing one. Drop in a 5-line minimal example so a contributor doesn't have to grep around — that's the trial-and-error this doc is meant to prevent. Inline.

[John Nguyen (johnguy0)]

I re-reviewed the delta since 91e4784. The earlier feedback is addressed. One new docs accuracy issue remains in the runner attributes table because it documents a client that is not present or wired. Not ready yet.

[John Nguyen (johnguy0)]

runner.pva is not wired today: cli.py only attaches graph and pp_admin, and this branch has no pva_client.py. Remove this row or add the client and runner wiring before using it in the guide.

[saengland]

Graham McMynn (@GrahamMcMynn) - confirmed: this branch has no pva_client.py (it lands in PR #37) and cli.py here only wires runner.graph + runner.pp_admin. There's a second doc-accuracy bug in the same table while we're here: runner.pp_admin is documented as PowerPlatformAdminClient, but the actual class in pp_admin_client.py is PPAdminClient.

Suggested fix:

 | `runner.graph` | `GraphClient \| None` | Microsoft Graph client |
-| `runner.pp_admin` | `PowerPlatformAdminClient \| None` | BAP admin client |
-| `runner.pva` | `PVAClient \| None` | Island Gateway client |
+| `runner.pp_admin` | `PPAdminClient \| None` | BAP admin client |
 | `runner.config` | `dict` | Parsed `my/config.json` |

And the matching reference at line 126:

-   same structure as `graph_client.py` / `pp_admin_client.py` / `pva_client.py`
+   same structure as `graph_client.py` / `pp_admin_client.py`

Add runner.pva and the pva_client.py reference back when PR #37 merges (or in the same merge if they go in together).

[Graham McMynn (GrahamMcMynn)]

I will not address this comment. The issue is I need to merge #37 first.

[saengland]

Reviewed AGENTS.md as the contract for future check authors. The structure (architecture, runner attributes, design principles, checklist) is right, but it documents the world that exists in PR #37, not the world that exists on main, and the security section has a critical gap. 5 findings:

#	Sev	Issue
1	HIGH	pva_client.py referenced as if it exists on main; only lives in PR #37
2	HIGH	runner.pva: PVAClient | None documented but cli.py never sets it
3	HIGH	Zero security guidance for the bug classes round-8 of PR #29 just fixed (defusedxml, retry adapter, CWE-209 scrubbing, HTTPS validation on operator URLs)
4	MEDIUM	Checklist step 4 doesn''t actually tell authors how to wire a new scope (FULL_SCOPE / SCOPE_MAP in cli.py)
5	LOW	12 em-dash chars throughout - kit convention is ASCII only

#1, #2, #3 are blockers. The security section gap is the biggest one - this guide will be the first thing the next check author reads, and it should encode the lessons we just paid for.

[saengland]

HIGH - Architecture table documents pva_client.py (Island Gateway client) as if it exists, but the file is not on main or in this PR's diff - it lives only on PR #37 (users/grahamc/260504UpdateFlightCheckerForNewChecks). A future check author following this guide before PR #37 merges will look for a file that isn't there. The same dangling reference also appears at lines 63-64 (See pva_client.py _discover_gateway() for the implementation pattern) and line 126 (lists pva_client.py as one of the existing client patterns to follow).

Fix: either (a) make PR #38 dependent on PR #37 and rebase it onto pr-37 so pva_client.py lands first, or (b) drop the Island Gateway row from the table and remove every pva_client.py / _discover_gateway reference (lines 16, 41-81, 63-64, 126) and add it back in a follow-up PR after PR #37 merges. Until then the only valid clients to document are auth.py, graph_client.py, and pp_admin_client.py.

[saengland]

HIGH - Runner attributes table claims runner.pva: PVAClient | None is available to check functions, but cli.py (lines 162-167 on main) only sets runner.config, runner.env_url, runner.dv_token, runner.env_id, runner.graph, runner.pp_admin. There is no runner.pva. The example at line 163 (# ... possibly using runner.graph / runner.pva / etc. ...) propagates the same error.

A check author who writes if not runner.pva: will hit AttributeError, not the documented None sentinel.

Fix: remove the runner.pva row from the table and the runner.pva mention from the example comment until the attribute is actually attached in cli.py (which happens in PR #37). After PR #37 merges, also document that tenant_id is implicitly needed by any new client following the existing MSAL pattern but is currently not exposed as a runner attribute - so authors must accept it as a separate arg or compute it from env_url.

[saengland]

HIGH - The Design Principles section (lines 99-141) has zero security guidance for the exact bug classes that landed in PR #29 round 8 (commits 97e9708, cb22c88):

1. xml.etree instead of defusedxml for any XML parsing -> XXE
2. No urllib3.util.retry.Retry adapter on requests.Session for transient 5xx/429 against BAP/Graph -> intermittent flake
3. Raw exception text echoed into CheckResult.result -> CWE-209 leak of bearer tokens / SAS URLs / PII when an upstream API fails
4. No scheme validation on operator-provided URLs (e.g., dataverseEndpoint from my/config.json) -> http:// or file:// could be accepted

Future check authors will repeat these because the guide doesn't warn them. This guide is the first thing the next contributor reads.

Fix: add a ## Security Requirements section after Design Principles with four bullets:

1. XML parsing: Use defusedxml.ElementTree not xml.etree for any XML payload (Dataverse SOAP, federation metadata). Catch both ET.ParseError AND defusedxml.common.DefusedXmlException so attack-path payloads (EntitiesForbidden, ExternalReferenceForbidden, DTDForbidden, NotSupportedError) fall through to a structured result instead of an unhandled traceback.
2. HTTP retry: Mount a urllib3.util.retry.Retry(total=3, backoff_factor=1, status_forcelist=[429,500,502,503,504], allowed_methods=GET/HEAD/OPTIONS only, respect_retry_after_header=True) adapter on a module-level requests.Session. Do NOT include POST/PUT/DELETE in allowed_methods - replays of mutating verbs on transient 5xx are unsafe.
3. CWE-209 scrubbing: Never put raw exception text or error_description into CheckResult.result or CheckResult.remediation. Bearer tokens, SAS URLs, and tenant IDs leak through requests.HTTPError.response.text and MSAL error_description. Echo only the error code and HTTP status; log full detail to stderr only.
4. URL validation: When accepting a URL from my/config.json or any operator-provided source, validate urlparse(url).scheme == "https" before sending bearer tokens to it. See auth._validate_https_url and pp_admin_client.derive_environment_id for the pattern.

[saengland]

MEDIUM - Checklist step 4 says "Wire it into _check_single_agent() (for per-agent checks) or the appropriate scope runner" but never tells the author HOW to wire a brand-new scope. The actual mechanism is: import the function in cli.py, add a tuple (category_name, function) to FULL_SCOPE, and add an entry to SCOPE_MAP if the check should also be runnable in isolation via --scope <name>. Without that, the runner.register(...) call never happens for the new check and it silently never executes.

Fix: replace step 4 with two explicit steps:

4. For per-agent checks (each agent under `my/agents/`), call your check from `_check_single_agent()` in `checks/local_files.py`.
5. For a new scope category, in `scripts/flightcheck/cli.py`:
   a. Import the check: `from flightcheck.checks.<module> import run_<name>_checks`
   b. Append `("<Category Name>", run_<name>_checks)` to `FULL_SCOPE`.
   c. If the scope should also run in isolation, add `"<scope_name>": [("<Category Name>", run_<name>_checks)]` to `SCOPE_MAP`.
   d. The runner picks it up via `runner.register(category, fn)`; no further wiring needed.

[saengland]

LOW - Document uses Unicode em-dash (U+2014) in 12 places: lines 1, 22, 33, 87, 105, 107, 108, 109, 111, 113, 115, 180. Rest of the kit's docs and source comments use ASCII hyphen. Mixed usage is a churn signal in git grep and breaks the kit's ASCII-only convention (Outlook COM mojibake history - same root cause when authors copy from rendered markdown into other surfaces).

Fix: replace every — with - (or -- if you want to preserve the visual weight). PowerShell from repo root:

(Get-Content scripts/flightcheck/AGENTS.md -Raw).Replace([char]0x2014, '-') | Set-Content scripts/flightcheck/AGENTS.md -NoNewline -Encoding utf8

[saengland]

Heads-up Graham McMynn (@GrahamMcMynn) Srikanth Deshpande (@srideshpande): retargeted this PR from main -> main2.

main2 is becoming the new default branch (the SFI-cleared post-rename history). main is being deprecated. All the PR #1-#10 stack and the rebrand have already landed on main2, and the existing main history will not survive the cutover.

Action needed on your side:

1. Rebase your branch on main2 (git fetch origin && git rebase origin/main2)
2. Resolve any conflicts (likely minimal - the trees diverged at file-layout level)
3. Force-push to your feature branch

Once rebased, the existing review comments stay attached and CI re-runs. If the rebase is messy, ping me and I'll help.

[saengland]

Reopened as #58 against main. The previous review (this PR) is preserved here for context.