Skip to content

Commit fc53fda

Browse files
priyanshu92Copilot
andauthored
Add code comment guidance to AGENTS.md (#196)
Adds a 'Code comments' subsection under Code Conventions that codifies WHY-focused commenting expectations for this repo's Node.js scripts and hooks. Covers documenting non-obvious reasoning, showing raw formats when parsing CLI/OData output, linking external standards (Dataverse status codes, OData error shapes), explaining privacy/telemetry behavior, and keeping workaround comments with tracking links. Examples are grounded in real repo patterns (pac auth banner parse, asyncoperations polling, telemetry scrubber). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0a9864c commit fc53fda

1 file changed

Lines changed: 99 additions & 0 deletions

File tree

AGENTS.md

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,105 @@ Current adopters: `power-pages`. Others adopt on demand.
7777

7878
**DRY (Don't Repeat Yourself):** Never duplicate logic across files. Each plugin has shared utilities (e.g., `scripts/lib/`) and shared reference docs (e.g., `references/`). Always check for and reuse existing helpers before writing new code. When adding shared logic, put it in the plugin's shared modules — not in individual skill directories.
7979

80+
### Code comments
81+
82+
Most code in this repo is Node.js scripts and hooks that shell out to `pac`/`az`, call the Dataverse and Power Platform APIs, and parse loosely structured CLI output. The reasoning behind a line is rarely obvious from the line alone, so comments matter.
83+
84+
* Err on the side of over-commenting code when the reasoning is not obvious. Comments should explain **WHY** code is written a particular way; the **WHY** is the most important part.
85+
* Do comment non-obvious implementation details: concurrency hazards, lifecycle constraints, compatibility requirements, platform quirks, upstream PAC CLI / Dataverse workarounds, and intentional deviations from the obvious helper or API.
86+
* When parsing strings, logs, CLI output, OData payloads, or other loosely structured data, include a comment with an example of the raw format being parsed. Show edge cases, escaping rules, delimiters, optional fields, or malformed-but-observed inputs when they affect the parser.
87+
* When code follows an external standard, protocol, or Power Platform convention (Dataverse status codes, OData error shapes, telemetry field contracts), include valid links to the relevant Microsoft Learn or specification source so future readers can verify the rule and understand why the code follows it.
88+
* When code touches telemetry, auth tokens, or anything privacy/security-sensitive, explain the scope, the opt-in/fail-closed behavior, and **why** — not just what it does.
89+
* Do not add comments that simply narrate clear code, such as "set the interval" immediately before assigning an interval.
90+
* Keep workaround comments close to the workaround. Include an issue link when the workaround is tied to an upstream bug, and describe the condition for removing it when that is known.
91+
92+
Good comments explain the constraint or tradeoff:
93+
94+
```javascript
95+
// `pac auth who` cold-starts the .NET runtime (~4s on Windows), so cache the parsed
96+
// result per process — repeated hook invocations must only fork the CLI once.
97+
let cachedAuth;
98+
```
99+
100+
```javascript
101+
// Refresh the bearer token roughly every 60s instead of on every poll. A long solution
102+
// export outlives the token's lifetime, but refreshing each 5s cycle would hammer the
103+
// az CLI for no benefit.
104+
const tokenRefreshEvery = Math.max(1, Math.floor(60000 / intervalMs));
105+
```
106+
107+
```javascript
108+
// Telemetry must never break the hook it runs inside, so this is fail-closed: a missing
109+
// executable, a timeout, or an unparseable banner all resolve to null rather than throw.
110+
return null;
111+
```
112+
113+
```javascript
114+
// Allowlist-only scrubbing: the event spec already restricts payload fields to values
115+
// that cannot carry PII, so this is a documented seam for a future regex pass — not a
116+
// no-op left unfinished by mistake.
117+
function scrub(value) {
118+
return value;
119+
}
120+
```
121+
122+
Code that follows an external standard or convention should link the source:
123+
124+
```javascript
125+
// Dataverse asyncoperations terminal states: statecode 3 (Completed) with statuscode 30
126+
// (Succeeded) means done; 31 (Failed) and 32 (Canceled) are the failure terminals.
127+
// See: https://learn.microsoft.com/en-us/power-apps/developer/data-platform/reference/entities/asyncoperation
128+
if (statecode === 3 && statuscode === 30) {
129+
return { status: 'Succeeded' };
130+
}
131+
```
132+
133+
Keep workaround comments next to the workaround and link the tracking issue:
134+
135+
```javascript
136+
// Workaround: `pac solution export` can exit 0 while the Dataverse async job is still
137+
// running, so ignore the exit code and poll asyncoperations to a terminal state instead.
138+
// Remove once the CLI blocks on the job result.
139+
// Tracking: https://github.com/microsoft/power-platform-skills/issues/1234 (use the real issue)
140+
const status = await pollAsyncOperation(asyncJobId, envUrl, token);
141+
```
142+
143+
Parsing comments should show the raw shape and important edge cases:
144+
145+
```javascript
146+
// Parse the `pac auth who` banner, a label/value block, e.g.:
147+
// Authority: https://login.microsoftonline.com/<tenant>
148+
// Tenant ID: 00000000-0000-0000-0000-000000000000
149+
// User: user@contoso.com
150+
// Values can themselves contain ':' (URLs), so match only up to the first colon after
151+
// the label, then trim. The JSON profile files are intentionally NOT parsed — that
152+
// format is internal and varies across PAC CLI versions.
153+
// `label` is a fixed, code-controlled string (e.g. 'Tenant ID'), so it is safe to
154+
// interpolate into the pattern. If a label ever comes from untrusted input, escape it
155+
// first to avoid regex injection.
156+
const re = new RegExp('^\\s*' + label + '\\s*:\\s*(\\S.*?)\\s*$', 'im');
157+
```
158+
159+
```javascript
160+
// Dataverse OData errors arrive as:
161+
// { "error": { "code": "0x80040217", "message": "..." } }
162+
// but some PAC surfaces capitalize the envelope as "Error", so check both before
163+
// falling back to plain-text pattern matching.
164+
const odataError = parsed.error || parsed.Error;
165+
```
166+
167+
Avoid comments that restate the code:
168+
169+
```javascript
170+
// Set the interval to five seconds.
171+
const intervalMs = 5000;
172+
173+
// Loop over the findings.
174+
for (const finding of findings) {
175+
report(finding);
176+
}
177+
```
178+
80179
## Maintaining This File
81180

82181
When you add new plugins or change the repository-level structure, update this file. For plugin-specific changes, update the plugin's own `AGENTS.md` (e.g., `plugins/power-pages/AGENTS.md`).

0 commit comments

Comments
 (0)