PR #3: Python scripts - #29
Conversation
Adds the toolkit's Python scripts under solutions/ess-agent-kit/scripts/: - auth.py - MSAL authentication helpers - checkpoint.py - state checkpointing for multi-step flows - discover.py - environment + agent component discovery - extract.py - extract agent components from Dataverse - fetch_and_setup.py - end-to-end onboarding orchestration - push.py - push topic/workflow changes back to Dataverse - setup.py - /setup command implementation - sync_docs.py, sync_samples.py - reference content sync - (additional helper scripts and utilities) SFI focus areas for this review: auth.py credential handling, fetch_and_setup.py orchestration, push.py write paths. Reviewer: @GrahamMcMynn Tracker: #10
Passwords containing &, <, >, or "" produced malformed SOAP XML and silent auth failures. Wrap username and password with xml.sax.saxutils.escape() in _build_soap_envelope().
rename solutions/ess-agent-kit -> solutions/ess-maker-skills
….local/+workspace/ rename This commit lands: - 3 of 5 P0 issues from the May 4 codebase analysis - Several P1 hardenings on auth.py - The path layout migration agreed in PR #2 (my/ -> workspace/+.local/) P0 - push.py partial-failure data loss (line 629 in pre-fix file): - Baseline + component_map are now updated only on errors == 0 (was: success > 0 unconditionally). Previously, if 5 of 10 components pushed and 5 failed, the baseline was refreshed for all 10, so the next push wouldn't see the failed 5 as changed and the customer's edits were silently lost. - save_component_map moved into the same gate; the in-memory map mutations during CREATE/DELETE no longer persist when there are errors. - main() now sys.exit(1) on any errors so /push wrappers detect failure. P0 - auth.py world-readable token cache (line 96 in pre-fix file): - MSAL token cache is now created with os.open(O_CREAT, 0o600) so the file is owner-readable only from the moment it exists. .local/ dir gets 0o700. Previously created with default umask (0o644) and refresh tokens were readable by any user on shared dev VMs - SFI blocker. P0 - setup.py non-atomic config.json write (line 540 in pre-fix file): - write_config now writes to .local/config.json.tmp and os.replace()s into place. A crash mid-write previously left a corrupted half-JSON file that bricked the kit (config.json gates every kit operation). P1 - auth.py: - HTTPS enforcement: discover_tenant/authenticate/query_all/update_record/ create_record/delete_record now reject env_url unless it starts with https://. Previously a misconfigured http:// URL would send Bearer tokens in cleartext. - explicit verify=True on every requests call (defense in depth against REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE tampering). - URL encoding via urllib.parse.quote on $select and $filter (was concatenated raw - any special char in agent name broke queries). - Don't echo MSAL error_description on failure (CWE-209) - it can include tenant IDs and internal flow details. Print only the error code. - 401 responses now raise AuthExpiredError instead of sys.exit so callers can re-authenticate without losing in-flight push state. P1 - push.py: - _AuthHolder + _call_with_refresh helper land for the long-push token refresh path. Long pushes (200+ components) can outlive an MSAL access token (~1 hour). Wiring of this helper across all 12 update/create/delete call sites is a follow-up commit on this branch. - Separate destructive-op confirmation: --yes covers creates and updates; deletes additionally require either --force-delete OR an interactive 'delete' confirmation typed by the user (kubectl-delete-style). P1 - graph_client.py + pp_admin_client.py: - Same 0o600 token cache hardening as auth.py. Path layout (per PR #2 review decision): - my/.token_cache.bin -> .local/.token_cache.bin (auth, graph, pp_admin) - my/config.json -> .local/config.json (auth.load_config, setup.write_config, flightcheck/cli, flightcheck/checks/workday, etc.) - my/agents/{slug}/ -> workspace/agents/{slug}/ (setup output_dir, fetch_and_setup, flightcheck/checks/local_files) - my/flightcheck/history/ -> workspace/flightcheck/history/ (flightcheck/runner default + cli default arg) - my/.component-map.json -> .local/.component-map.json (string refs only; push.py still loads it from agent_dir per its actual on-disk location) Total: 30 my/ -> .local/+workspace/ renames across 10 files. Follow-ups (separate commits on this branch): - Wire _call_with_refresh through all 12 push.py call sites - etag/If-Match support in push.py update calls (auth.update_record now accepts an etag arg; push.py is not yet passing one) - Add unit tests for compute_diff, _call_with_refresh, atomic write
CodeQL: Clear-text logging of sensitive information (github-advanced-security[bot] alert #2 on workday.py:423, 7 instances): - _soap_call previously returned `resp.text[:500]` as the error string. On error responses Workday can echo parts of the request envelope back, and the request envelope contains the WS-Security UsernameToken (ISU password in cleartext). The error string then flowed through CheckResult.result into the FlightCheck JSON / HTML report. Fix: - New _redact_ws_security() helper strips any Security / UsernameToken / Password XML block via regex before any return-to-caller path. - New _summarize_soap_error() parses the SOAP fault to extract just the faultstring (which describes the error condition without echoing the request body); falls back to bare HTTP status code on parse failure. Uses defusedxml (already added in PR #14) to avoid entity expansion. - _soap_call now: * Returns the redacted response on success (defense in depth - success bodies normally don't contain WS-Security but be cautious) * Returns _summarize_soap_error(...) on HTTP >= 400 instead of raw resp.text * Strips the password from str(e) on exception paths - httpx.Client now opens with follow_redirects=False so a 302 from Workday cannot replay the WS-Security header to an attacker-controlled host. setup.py:38 (HIGH per johnguy0 review): - Removed the trailing "# Remove the import above" instruction-to-self comment. The `from datetime import date` import is kept (date.today() is used at line 366).
The CodeQL Clear-text logging rule (py/clear-text-logging-sensitive-data) was still firing on workday.py:423 after a5cb8ab. Root cause: tuple-unpacking from a single resolver that returns both metadata and password causes CodeQL to taint every output of the call - so wd_tenant inherited the 'private' tag from wd_password, and printing the tenant name was flagged as clear-text logging. Fix: split the resolver into two: - _resolve_workday_metadata(runner) -> (base_url, tenant, test_employee). No sensitive data ever returned, so CodeQL doesn't taint the outputs. - _resolve_workday_credentials(runner, tenant) -> (username, password). Only sensitive data, no metadata - so password taint can't propagate to URL/tenant vars in the caller. _check_workflows now: 1. Resolves metadata first. 2. Validates + prints the tenant status line (safe - no creds in scope yet). 3. Resolves credentials only after the print, so the print is upstream of the sensitive bind point on the data-flow graph. Legacy _resolve_workday_creds kept as a compatibility shim that composes the two new functions, in case any external caller or test still uses the 5-tuple.
… fix v3) CodeQL classifies any WORKDAY_* env var as private/sensitive, so even printing the tenant name is flagged as clear-text logging of sensitive information (rule py/clear-text-logging-sensitive-data, 5+ flow paths). Splitting the resolver in 4bf773e wasn't enough - the issue is the print statements themselves, not the call graph. Fix: remove tenant interpolation from all print() calls in this module: - _check_workflows status line: `Testing 17 Workday workflows...` (no tenant) - _resolve_workday_credentials prompt: drop the `Tenant: {tenant}` line - Username @tenant assembly: switch from f-string to concat (semantically identical, but CodeQL's f-string sink detection may differ) The tenant is still used to construct the SOAP service URL inside _soap_call (line 782) - that is URL construction, not logging, and not flagged by the rule. User-visible: status line is slightly less informative, but the tenant already appears in the eventual SOAP URL on any successful call so it isn't actually hidden from the user.
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Round 1 on PR #29 (= round 2 on the underlying review/03-scripts work).
Context: PR #13 was prematurely merged earlier today and rolled back; this re-opens the same branch with 4 follow-up commits addressing the PR #13 review.
All 4 CRITICAL findings from PR #13 are addressed:
- ✅
push.py:629— partial-failure baseline now correctly gated:if errors == 0 and success > 0:. Explicit user-facing warning when errors > 0 ("Baseline NOT updated"). The in-source comment explaining the rationale is good craft. - ✅
push.py:625— component_map saved in the sameerrors == 0block. - ✅
auth.pytoken cache file — usesos.openwith explicit0o600mode (avoids the umask race). Defense-in-depthos.chmodafter, with Windows fallback. - ✅
auth.pycache directory —os.chmod(LOCAL_STATE_DIR, 0o700)after makedirs.
Most HIGH findings addressed:
- ✅
setup.py:38— unfinished refactor leftover gone. - ✅
--force-deleteflag added to push.py with interactivetype 'delete' to confirm. Stronger than my recommendation: even with--yes, deletes require--force-delete. Good. - ✅ Token refresh during long pushes —
AuthExpiredErrorraised on 401,_call_with_refreshwrapper in push.py catches, re-authenticates, retries once. ⚠️ Etag/optimistic concurrency — plumbed inauth.py:182but not wired up in any of the 4update_recordcall sites in push.py. Half-built. Either pass cached etag values or remove the parameter to keep the API honest.- ❌
requirements.txt/pyproject.toml— still missing. No pip manifest anywhere in the tree. CI (per PR #11) doesn'tpip installso contributors can't reproducibly run the scripts. Dependabot pip ecosystem has nothing to track. This was HIGH in round 1 and is still HIGH.
Most MEDIUM findings addressed:
- ✅ HTTPS validation —
_validate_https_urlrejects http:// - ✅
verify=Trueexplicit on everyrequests.*call - ✅ OData filter URL-encoded via
urllib.parse.quote - ✅ Atomic
config.jsonwrite (write-tmp-then-os.replace) - ✅ Error message scrubbing —
error_descriptionno longer echoed (CWE-209 closed) ⚠️ setup.pyidempotency —--refreshflag exists, but normal mode without--refreshstill silently overwrites the agent folder. Theif args.refresh and os.path.exists(output_dir):check at the top means: with--refresh, checkpoint then proceed; without--refresh, just proceed and overwrite. The right fallback is to detect an existing folder in normal mode and refuse without--refresh.- ❌
configVersionfield onconfig.json— not added. First time we change the schema, every existing install KeyErrors. - ❌ Pre-push schema validation — push.py still sends file content directly to Dataverse without YAML/JSON parse-checks. Cryptic errors on malformed input.
- ❌ 429 / 5xx retry-with-backoff —
auth.pyhas no retry on transient errors. One transient 503 mid-push leaves the customer in partial state (which compounds because the partial-failure gating only catches what fails fully, not mid-flight transient retries).
Bonus work I didn't ask for:
- CodeQL clear-text logging alert addressed via splitting
_resolve_workday_credentialsfrom_resolve_workday_metadatato break the taint chain (commits4bf773ef,e5dd667a). Clean fix that preserves user-facing tenant info in messages without tainting the credential path.
LOW (carry-over):
auth.py:32CLIENT_ID still undocumented ("Well-known first-party client ID for Power Platform / Dynamics tools" without a source link). One-line fix.
PR-level:
- PR description is the PR #13 description verbatim, including the
solutions/ess-agent-kit/scripts/path reference. Same stale-rename issue. Refresh before merge — 9th PR with this issue (PRs #2-#5, #7-#9, #28, #29).
Inline below for the open items that anchor cleanly. Recommend hold on the missing requirements.txt and the half-built etag — both are quick fixes and both are real defects. Idempotency, configVersion, retry/backoff, pre-push validation, and CLIENT_ID docs are nice-to-have round-3 follow-ups.
12 ruff violations surfaced now that PR #1 made lint a real CI gate (removed continue-on-error). All edits are mechanical and behavior-preserving: scripts/extract.py:17 - E401: split 'import json, os, re, sys' into 4 separate import lines per PEP 8. scripts/flightcheck/checks/local_files.py:12 - F401: drop unused 'import os'. scripts/flightcheck/checks/publishing.py:10 - F401: drop unused 'Priority' from runner import. Other names (CheckResult, Status) still in use. scripts/flightcheck/graph_client.py:13 - F401: drop unused 'import json' (resp.json() is requests' built-in, not the stdlib module). scripts/flightcheck/pp_admin_client.py:13,15 - F401 x2: drop unused 'import json' and 'import re'. scripts/push.py:179-180 - F541 x2: removed 'f' prefix from string-concat literals that have no placeholders. scripts/setup.py:309,313,314,317 - F541 x4: removed 'f' prefix from 4 placeholder-less metadata-yaml literals; the surrounding lines that DO use placeholders keep their 'f' prefix. Note: the lint job will still report 1 E902 'No such file or directory' for solutions/ess-maker-skills/src/mcp/ - that path lands in PRs #4/#5 and is a separate cross-branch CI issue (tracked separately, not part of this commit).
|
John Nguyen (@johnguy0) — heads up: the remaining red X on Lint Python here is the cross-branch E902 ( |
Blockers from PR #29 round-2 review: * Add scripts/requirements.txt with pinned msal/requests/urllib3/PyYAML so contributors can pip-install reproducibly. Dependabot pip ecosystem can now track. Was HIGH in rounds 1-2. * auth.py update_record(): drop the etag parameter (and If-Match block). It was plumbed but never wired up at any of the 4 push.py call sites; half-built protection is worse than no protection. Re-add when push.py is ready to capture and pass @odata.etag from the original GET. SFI/MSRC concern about silently-failing controls addressed. Round-3 follow-ups (also from review): * setup.py idempotency: refuse silent overwrite when output_dir already exists; require --refresh explicitly. Belt-and-suspenders with the prompt orchestration in PR #18. * config.json: add 'configVersion: 1' so future schema changes can gate cleanly without KeyError on existing installs. * auth.py: bounded retry-with-backoff for 429/5xx via urllib3.util.retry.Retry mounted on a module-level Session. Power Platform throttles; one transient 503 mid-push otherwise leaves the customer in partial state and compounds with the partial-failure baseline gating in push.py. Status forcelist 429/500/502/503/504, 3 retries, backoff_factor=1, respects Retry-After header, all HTTP verbs in scope. * push.py: pre-push schema validation - YAML safe_load on .mcs.yml/workflow-meta, json.loads on workflow.json + JSON-shaped template configs. Surfaces a clear local error before mid-push. yaml import is guarded so PyYAML stays optional at import time (CI installs it from requirements.txt). * auth.py: CLIENT_ID now documented with the learn.microsoft.com source link and scope explanation. SFI/MSRC reviewers don't have to ask. All edits behavior-preserving except where they're explicitly tightening behavior (idempotency, schema validation). Lint clean (ruff).
|
John Nguyen (@johnguy0) — round-3 fixes pushed (commit Blockers
Round-3 follow-ups
PR description refreshed too (now correctly references |
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Round 2 on PR #29 (= round 3 on the underlying review/03-scripts work).
The round-3 commit (624cd2cd) is well-targeted at the round-2 findings. Verified addressed:
requirements.txtadded with pinned msal/requests/urllib3/PyYAML. Dependabot pip ecosystem can now track. Closes the round-1/2 HIGH.update_recordetag parameter dropped. Comment in the docstring explains the rationale and the re-add path. Half-built control removed, good.- Bounded retry-with-backoff via
urllib3.util.retry.Retrymounted on a module-level_SESSION. Status forcelist 429/500/502/503/504, 3 retries, backoff_factor=1, respects Retry-After. Verified all six Dataverse helpers swapped fromrequests.*to_SESSION.*. Closes the round-2 MEDIUM. setup.pyidempotency now gated by existence, not by--refreshflag. Refuses silent overwrite. Belt-and-suspenders with the prompt orchestration in PR #18.- Pre-push schema validation added: yaml.safe_load on .mcs.yml/workflow-meta, json.loads on workflow.json + JSON-shaped template configs. Surfaces clear local errors before mid-push. yaml import guarded so PyYAML stays optional at import time.
- CLIENT_ID documented with learn.microsoft.com source link and scope explanation.
configVersion: 1field added to.local/config.json.
New CRITICAL found in this round:
The _call_with_refresh helper and _AuthHolder class in push.py are dead code. Defined, never called. Every push CRUD call site bypasses the wrapper. This was checked off in round 2 based on the helper existing; the wiring isn't there. Long pushes (>1hr token TTL) will still fail with no auto-recovery. Inline below at the helper definition with the fix.
New HIGH found in this round:
-
Eval child→parent assignment uses dict iteration order with a
breakthat admits "(usually only)". With multiple new eval sets in one push, every child gets attached to the first parent, silently corrupting evaluation hierarchies. Inline at line 626. -
The
errors == 0 and success > 0gate protects the file write but the delete loop already mutatedcomponent_mapin memory. Partial-failure delete leaves the in-memory map and on-disk map out of sync, and the next/pushretries already-deleted records, never converges. The round-2 CRITICAL fix is incomplete. Inline at the gate.
MEDIUM:
- Workflow-meta is YAML-validated by the new pre-push step, then parsed by hand-rolled
startswithin the actual push code. Use yaml.safe_load both places. Inline at line 420.
LOW (carry-over / round-4 follow-ups):
- xml_escape
employee_idandresponse_groupin the Workday SOAP body builders. Defense-in-depth, narrow exposure today. Inline. - configVersion written but not read. Track read-side gate as round-4 follow-up. Inline.
Recommend hold on the CRITICAL and the two HIGHs. All three are real defects, all three have concrete fixes, all three are quick. The MEDIUM and LOWs can land in round 4.
Self-review note for the round-2 reviewer (me): the _call_with_refresh finding should have been caught by grepping for actual call sites of the helper, not just verifying the helper exists. Adding "verify the helper has callers" to my Python-script review checklist.
- John
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Round 3 review addendum: rubber-duck pass surfaced two real defects I missed and one correction to my earlier finding.
Correction: finding 5 ("xml_escape employee_id and response_group") was partially wrong. response_group is intentionally a raw static XML fragment from the WORKFLOWS table (e.g., <bsvc:Include_Reference>true</bsvc:Include_Reference>); escaping it would corrupt the envelope. employee_id escaping is still correct. Comment edited inline.
Two new findings:
-
HIGH (create-side): The same partial-failure gate that wedges deletes also re-creates records on rerun. New files are POSTed but the baseline isn't updated on partial failure, so the next push sees them as
newagain and re-POSTs. Bot components withschemanameuniqueness may catch some of this; template configs and workflows don't have the same server-side guard. Inline at the create-loop save gate. -
CRITICAL (regression introduced in round 3):
_RETRY.allowed_methodsincludes PATCH/POST/DELETE. urllib3 will replay POSTs on 429/5xx, which can duplicate records when the first request landed but the response packet was lost. This is the textbook non-idempotent-retry trap. Drop PATCH/POST/DELETE fromallowed_methodsand handle write-side throttling at the call site with explicit logic. Inline.
MEDIUM (tracking, not blocking): etag/If-Match concurrency is gone after round 3 (deliberately). Document the roadmap: persist @odata.etag in .component-map.json during setup, re-add the parameter, wire the call sites, surface 412. Inline.
Net: the gate-on-full-success model is correct in concept; the round-3 implementation only protects the file write, not the in-memory state, and not the network-level retry semantics. Small surgical fixes on each.
- John
CRITICAL * push.py: wire up _call_with_refresh at all 12 CRUD call sites (lines 355, 382, 398, 430, 475, 506, 545, 594, 639, 682, 697, 717). The helper + _AuthHolder were dead code in round 3 - defined but never called - so any push that outlived the MSAL access token TTL (~1hr) would die at the first call after expiry with no auto-recovery. Drop the local 'token' variable, pass auth.token at every call site so the loop re-reads after refresh and the helper's positional-arg mutation hack works correctly for the in-flight retry. Caller-side detection of 401 still raises AuthExpiredError in auth.py; the wrapper catches and retries with a fresh token. * auth.py: restrict _RETRY.allowed_methods to read-only verbs (GET/HEAD/OPTIONS only). urllib3 was replaying PATCH/POST/DELETE on 429/5xx, which can duplicate records when the first request landed but the response packet was lost (botcomponents are guarded by schemaname uniqueness, but template configs and workflows have no server-side guard). This is the textbook non-idempotent-retry trap and was a regression introduced in round 3 by adding the Retry adapter without restricting the verb set. Mutating verbs now bypass the auto-retry; the call-site try/except handles their throttling explicitly. HIGH * push.py eval child to parent: replace 'first parent in dict.items() with break' with folder-path matching. With multiple new evaluation sets pushed in one run, every child got attached to the first parent in iteration order, silently corrupting evaluation hierarchies. Now matches by evaluations/<set-name>/ folder; existing parentbotcomponentid in component_map still wins as before. * push.py partial-failure gate (delete side): track deletes in pending_deletes side-set; apply only inside 'errors == 0 and success > 0' block. Round 3 protected the file write but mutated component_map in-loop via 'del component_map[filepath]'; on partial delete (5/6 succeed) the in-memory map dropped 5 entries and lost them on process exit, while the on-disk map still had all 6, leading to the next push retrying already-completed deletes against missing records and never converging. * push.py partial-failure gate (create side): same shape as deletes - track creates in pending_creates side-dict; defer template-config meta-file writes (with the new templateconfigid stamped in) into pending_meta_writes; apply all three only inside the success gate. Round 3 left the create-side wedge open: a partial create (5/6 succeed) left the baseline unchanged so the next push re-POSTed all 6 originals; bot components were saved by schemaname uniqueness but template configs and workflows duplicated cleanly. MEDIUM * push.py workflow-meta parsing: replace hand-rolled startswith() splits with yaml.safe_load (both at the update path around line 414 and the create path around line 512). The pre-push schema validation already YAML-parses these files; reusing PyYAML in the actual push path keeps the two consistent and fixes silent breakage on quoted values, leading whitespace, multi-line block scalars, and inline comments. PyYAML is now imported at module level (it is required, not optional - it is in scripts/requirements.txt and the pre-push validation step calls it unconditionally). LOW * flightcheck/checks/workday.py: xml_escape employee_id and effective_date in the three SOAP body builders (_build_get_workers_body, _build_compensation_body, _build_write_test_body). Defense-in-depth: these values come from .local/config.json or env vars, are usually numeric, but a typoed config (e.g. an & in an employee ID) used to produce a malformed envelope that surfaced as a cryptic SOAP fault rather than a clear local error. response_group is intentionally NOT escaped - it is a static XML fragment from the WORKFLOWS table by design (e.g. '<bsvc:Include_Reference>true</bsvc:Include_Reference>'); escaping it would corrupt the envelope. Per round-3 reviewer correction. TRACKED FOLLOW-UPS (not in this commit, per reviewer note) * configVersion read-side gate in auth.load_config (currently written but ignored). Adds a hard error when the on-disk schema version no longer matches what the running scripts expect. * etag/If-Match optimistic concurrency: persist @odata.etag in .component-map.json during /setup, re-add the etag parameter to update_record / delete_record, wire all call sites, surface 412 with a 'remote was edited, run /setup --refresh' message. Lint clean (ruff). All edits behavior-preserving for the supported call paths; tightening on the abuse paths.
Same slip as the cleanup commit on review/05-mcp-servicenow earlier today: 'git add -A' picked up __pycache__ files generated by a local 'python -m py_compile' lint check. .gitignore already excludes __pycache__/ globally; these slipped in from outside the gitignore root. Removing now and adding a stricter local pre-commit habit.
|
John Nguyen (@johnguy0) — round-4 fixes pushed (commit All 5 blockers from your two round-3 reviews addressed: CRITICAL
HIGH
MEDIUM
LOW
Tracked follow-ups (not in this commit, per your guidance)
Self-correction noted: I marked the round-2 token-refresh item as ✅ based on the wrapper existing without grepping for callers - the same anti-pattern as etag in round 1. Adding "verify the helper has callers" to my own round-N self-review checklist. Ready for your re-review. |
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Round 4 review (= round 5 on review/03-scripts).
The round-4 commit (17155522) is comprehensive and well-targeted. Verified:
- All 12 CRUD call sites in push.py wrapped via
_call_with_refresh. Localtokendropped,auth.tokenre-read each call. Closes round-3 CRITICAL. _RETRY.allowed_methodsrestricted to GET/HEAD/OPTIONS only. Mutations no longer auto-replayed on 429/5xx. Closes round-3 CRITICAL (regression introduced earlier in round 3).- Side-collections (
pending_creates,pending_deletes,pending_meta_writes) introduced. Component_map mutations and template-config meta-file writes deferred to theerrors == 0 and success > 0gate. Closes round-3 HIGH (delete side) and HIGH (create side). - workflow-meta parsed via
yaml.safe_loadin both update and create paths. PyYAML now a hard requirement, imported at module level. Closes round-3 MEDIUM. xml_escapeapplied toemployee_idandeffective_datein all three Workday SOAP body builders.response_groupcorrectly left raw with explanatory comment. Closes round-3 LOW with the response_group correction baked in.- configVersion read-side gate and etag/If-Match concurrency explicitly tracked as round-5 follow-ups in the commit message. Acceptable scope.
The __pycache__ slip in 17155522 was caught and reverted in d92bcc1b. Process slip, not a defect. Author acknowledged the gitignore-root issue in the cleanup commit.
One blocking finding from this round (HIGH):
The eval child to parent resolution still produces an orphan in the most common workflow: a customer adds a new test case to an evaluation set whose parent already exists remotely. Neither lookup path finds the parent (component_map.get(filepath) looks up the child's path, eval_parent_ids only contains parents being newly created in this push). Result: child created with no ParentBotComponentId@odata.bind, broken hierarchy in Dataverse. Round 3 silently mis-attached; round 4 silently orphans. Inline below with a fail-closed fix.
Two LOW follow-ups, not blocking:
- The success gate is sequenced but not transactional. A mid-gate disk-write failure (meta files, component_map save) reopens the create-side wedge that round 4 just closed for the API side. Wrap the gate or write-tmp-then-os.replace. Inline.
- One in-loop
entry["name"] = record["name"]mutation slipped through the side-set conversion. Benign in practice (idempotent on retry) but breaks the "all state staged for the gate" contract. Stage aspending_renames. Inline.
Recommend hold on the eval orphan case. Trivial to fix, real customer-visible consequence. The two LOWs can land in round 5 alongside.
Self-review: I missed the eval orphan case in my own walkthrough of the round-4 diff. Caught by a rubber-duck pass. The pattern of "round-N silently does X wrong, round-N+1 silently does X' wrong" tells me my eval-section reading checklist needs to add: "for every parent-child resolution path, walk through (a) re-push of existing child, (b) new child + new parent in same push, (c) new child + pre-existing parent." I had only carried forward (b) from round 3.
- John
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Two scope / naming concerns on this PR that are separate from the round-3 / round-4 correctness work.
1. sync_docs.py and sync_samples.py don't belong in the customer-facing scripts/ folder. Both are maintainer-side utilities that fetch upstream MS Learn / Copilot Samples content into src/reference/ and src/examples/. Customers cloning this repo already have those copies. Shipping the sync scripts in scripts/ invites customers to run them, which hits the GitHub anonymous rate limit, silently overwrites any local doc edits, and couples customer setup to upstream branch health. Move to maintainer tooling (CI job that opens a refresh PR), or out of scripts/ into a tools/ / internal/ location with a clear "kit maintainers only" README, or delete and refresh the vendored content by hand-PR. Inline on both files.
2. "ESS Copilot Kit" is sprinkled across 16 files as if it were a settled product name, but it doesn't match anything anchored. The repo is Employee-Self-Service-Agent-Developer-Kit, the folder is solutions/ess-maker-skills/, the product is the "Employee Self-Service agent" in M365 Copilot, and "Copilot Kit" is a separate OSS project that's a confusion risk. Pick one name and apply it consistently before customers start citing the kit by whatever name they see first. Inline on auth.py with the file list and rationale.
Neither is a CRUD-correctness blocker, but the scope concern (sync scripts in customer space) and the naming drift compound over time and get harder to fix once external customers anchor on whatever they see first. Worth landing a tightening pass alongside the eval-orphan fix from my prior review.
- John
… drop sync scripts (slice PR #3) HIGH * push.py eval child-to-parent: round 4 fixed mis-attach but introduced silent orphans. The two existing lookup paths only matched (a) re-pushed children whose stored parentbotcomponentid is in component_map, and (b) parents being newly created in this push run. The most common case - a customer adds a new test case under an evaluation set whose parent already exists remotely - missed both paths and shipped a CREATE with no ParentBotComponentId@odata.bind. Round 5 adds a third lookup: scan component_map for componenttype==19 entries that are themselves parents (no parentbotcomponentid) and live in the same evaluations/<set-name>/ folder as the child. If all three lookups miss, fail closed with a clear error - do NOT create the orphan and let the customer fix it in Maker Studio after the fact. LOW * push.py atomic-ish gate: wrap the success-gate body (component_map mutations, meta-file writes, save_component_map, update_baseline) in try/except OSError. If a disk-write fails mid-gate after API operations have already committed remotely, surface a clear 'remote committed but local stale, run /setup --refresh' message and exit non-zero (sys.exit(3)). Tracked follow-up to land a true atomic write via *.tmp + os.replace for the meta and map files; for now the explicit recovery message keeps the customer un-wedged. * push.py pending_renames: stage workflow-meta name updates instead of mutating component_map[filepath]['name'] in-loop. Round 4 moved every other CRUD-side mutation into pending_creates / pending_deletes / pending_meta_writes; this rename slipped because it lives inside a workflow-meta update branch rather than as a CRUD on component_map directly. Apply at the gate alongside the others for consistency. Failure mode was benign in practice (idempotent on retry) but breaks the 'all state staged for the gate' contract. SCOPE * Delete scripts/sync_docs.py and scripts/sync_samples.py. These are kit-maintainer utilities that fetch upstream MS Learn / Copilot Samples content into src/reference/ and src/examples/. Customers cloning this repo already have those copies; running an unauthenticated GitHub API fetch as part of customer setup hits the anonymous rate limit (60 req/hr per IP, fails on corp NAT after a few users), silently overwrites local doc edits via clean_content's idempotent rewrite, and couples customer setup time to upstream branch health. Refresh of vendored content moves to maintainer hand-PR going forward. No callers in solutions/ - the deletion is clean. TRACKED FOLLOW-UPS (per reviewer guidance) * configVersion read-side gate in auth.load_config * etag/If-Match concurrency: persist @odata.etag in .component-map.json during /setup, re-add etag param, wire call sites, surface 412 * Atomic local persist via *.tmp + os.replace pattern (already used by setup.py for config.json; reuse the shape) NAMING (separate response in PR comments): 'ESS Copilot Kit' brand IS already anchored in the top-level README and CONTRIBUTING.md (per the round-2 review on PR #11 that explicitly approved 'README header ESS Copilot Kit matches CONTRIBUTING + OSS Portal review #55042'). The 16 file headers and 2 user-facing strings in this PR are deliberately consistent with that decision. No code change needed; documenting the anchor for John in the PR thread. Lint clean (ruff). Behavior-preserving except for the deliberate fail-closed eval-orphan path.
|
John Nguyen (@johnguy0) — round-5 fixes pushed (commit HIGH (blocking)
LOW
SCOPE
Tracked follow-ups (per your guidance, not in this commit)
On naming ("ESS Copilot Kit"): worth a quick alignment check — the brand IS anchored, just not in this PR's diff. The round-2 review on PR #11 ( That said, I take your point about the OSS-portal
Sam owns the call. Will follow up here once decided. For now leaving the strings as-is so we don't churn 16 files twice. Self-correction: I missed the eval-orphan case in my own walkthrough of the round-4 diff (same anti-pattern as the dead-helper miss in round-2). Adding "for every parent-child resolution path, walk through (a) re-push of existing child, (b) new child + new parent, (c) new child + pre-existing parent" to my own checklist. Caught here by your rubber-duck pass; appreciated. Ready for your re-review. |
…trings (slice PR #3) Inside-repo rename per the naming policy locked in chore PR (rebrand outside-repo / inside-repo split). 14 files in scripts/: auth.py, checkpoint.py, discover.py, extract.py, fetch_and_setup.py, push.py, setup.py, requirements.txt header comment, and the flightcheck/ tree (__init__.py x2, cli.py, graph_client.py, pp_admin_client.py, runner.py). Companion to chore PR for the outside-repo files (root README, ISSUE_TEMPLATE/*) and the inner solution README. No code changes; doc/string only.
Adds src/reference/ess-docs/README.md documenting the snapshot policy: why the docs are vendored (Copilot Chat grounding), source URL, snapshot date (2026-04-29), refresh policy (hand-PR; the auto-sync script was removed in slice PR #3 for corp-NAT rate-limit + silent-overwrite reasons), and explicit non-goals (not a fork, not the place for kit-specific guidance). Addresses the 'feels wasteful to be cloning those' concern raised by @johnguy0 in the PR #29 review thread. The duplication is the explicit tradeoff that gives the kit its domain-correct first-turn output quality; the snapshot stamp makes the staleness contract explicit. Companion: src/examples/ess-samples/ gets an analogous README in slice PR #10.
|
John Nguyen (@johnguy0) — closing the loop on the two scope/naming items from your review across all open PRs: 1. Naming locked: "Employee Self-Service Agent Developer Kit" (outside) / "ESS Maker Kit" (inside) After offline alignment with Sam: outside the inner solution (repo identity files) we use the full repo name; inside the inner solution (the surface customers interact with) we use "ESS Maker Kit". This drops the prior "ESS Copilot Kit" label entirely, which closes your Rebrand commits across the open PRs:
2. Vendored docs/samples: keep them, document the contract After offline alignment with Sam: we agree the vendoring is wasteful in the abstract, but the kit's first-turn quality on Three additions to make the contract explicit instead of implicit:
Sam will validate first-turn agent quality with the vendored content in dev tenant after the stack ships; if the data shows the grounding edge is small enough to delete the vendored copies, that becomes a follow-up PR. For now the snapshot stamps make the staleness contract explicit. PR #41 is the smallest blast radius - safe to land first; it sets the brand baseline for everything else. The companion rebrand commits in the existing review/* PRs all follow the same policy. Ready for your re-review. |
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Reviewed the two commits since d92bcc1. They remove the sync scripts, rename script headers to ESS Maker Kit, and rework push.py's eval parent and persist gate handling. The staged workflow rename uses the metadata path instead of the component-map key, so one correctness issue remains. Not ready yet.
… refresh stale eval-lookup comment (slice PR #3) Addresses johnguy0 round-5 review on PR #29 (commit f66c6b0). 1. HIGH: pending_renames staged the metadata.yml path, but component_map tracks workflow entries by workflow.json path. Successful renames PATCHed Dataverse but were dropped at the gate, leaving the on-disk map with the old name. Key by wf_json_path. 2. LOW (atomicity): success gate now does a true two-phase persist. Phase 1 mutates the in-memory map; Phase 2 writes every artifact to a *.tmp sibling (with cleanup on failure); Phase 3 os.replace's all tmps into place with component_map.json renamed last; Phase 4 is the best-effort baseline copy. save_component_map and a new _atomic_write_text helper share the *.tmp + os.replace pattern from setup.write_config. 3. MEDIUM (stale comment): the eval-lookup comment block carried the round-3 dict-iteration narrative on top of the round-5 fail-closed description. Replaced with the actual three-path priority order so the next edit doesn't have to reconcile two stories.
…29-#31) (#47) * ci: lint only the Python paths that actually exist on the branch Old workflow hard-coded both 'scripts/' and 'src/mcp/' in the ruff and compileall args. ruff's E902 fires whenever either path is missing, so the lint job fails on branches that don't include the full kit yet: - review/03-scripts (PR #29) has scripts/ but no src/mcp/ -> fail - review/04-mcp-workday (PR #30) has scripts/ + src/mcp/workday/ but ruff was invoked against the parent src/mcp/ which is fine, except the prior gate also expected both legs. - review/05-mcp-servicenow (PR #31): same shape as PR #30. Replace the binary has_python gate with a discovery step that only adds each candidate directory to the lint/compile path list when that directory exists AND contains .py files. Both downstream steps then run against exactly the dirs present on the branch. * ci: address PR #47 review (env-var indirection + slice-PR hint) Two findings from @johnguy0: 1. The `paths` output interpolated directly into the shell command in the Lint and Check Python syntax steps is the classic GHA expression-injection shape (CodeQL actions/command-injection). Safe today because the candidate list is hard-coded with no metacharacters, but it is the pattern that causes the next contributor accident the moment someone adds a variable-derived or PR-controlled candidate. Pass paths through env: PATHS and reference $PATHS in the run script instead. 2. Skip-path notice lost the specific "Python lands in PR #3-#5" hint that helps anyone debugging a green-but-skipped lint job while the slice PRs are still in flight. Restore the slice-PR mention.
#1 (LOW: defense-in-depth xml_escape) - already fixed in a prior round. All three SOAP body builders in workday.py (_build_get_workers_body, _build_compensation_body, _build_write_test_body) already call xml_escape(employee_id). response_group is intentionally left raw (per John's correction) because the WORKFLOWS table stores it as static XML fragments. No code change here, just resolving the thread. #2 (LOW: configVersion read-side gate) - fixed: - auth.py: define EXPECTED_CONFIG_VERSION = 1 alongside LOCAL_STATE_DIR. load_config() now reads cfg.get("configVersion", 0) and exits with a clear "schema vN, expected vM, run /setup --refresh to migrate" message on mismatch. - setup.py: import EXPECTED_CONFIG_VERSION from auth, stamp it into write_config (instead of the hardcoded 1), and print a NOTE when the existing on-disk version differs so the operator knows /setup just rewrote the schema. Future migrations that need field-level transforms can branch on existing_version before merging. #3 (MEDIUM: concurrency control roadmap) - filed as #51: John explicitly flagged this as "not blocking on this PR, documenting so it doesn't get lost." Issue #51 captures the roadmap (capture etags in setup -> wire If-Match into update_record / delete_record -> surface 412 with /setup --refresh hint).
|
John Nguyen (@johnguy0) - round-7 follow-ups landed in
CI is green now (PR #47 ci.yml fix landed in main2 and the re-run picked it up). |
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Round 7 follow-ups all check out.
- xml_escape: confirmed
employee_idis escaped in_build_get_workers_body,_build_compensation_body, and_build_write_test_body.response_groupcorrectly left raw. - configVersion read-side gate:
EXPECTED_CONFIG_VERSIONlives inauth.py,load_config()exits cleanly on mismatch with a/setup --refreshinstruction, andsetup.pystamps the constant and prints a migration NOTE when versions differ. - Concurrency control: tracked as #51 with the full roadmap.
All three remaining threads resolved. Nothing else from me. Ready for approval.
Round-8 review: success-path used stdlib xml.etree.ElementTree.fromstring while error-path already used defusedxml. Standardize on defusedxml at module level; drop inline import. defusedxml.ElementTree.ParseError is a subclass of stdlib ET.ParseError so existing except-handlers stay correct. Pin defusedxml>=0.7.1,<0.8 in requirements.txt.
Srikanth Deshpande (srideshpande)
left a comment
There was a problem hiding this comment.
These are reviewed by John's agent, and per the agent, it is ready for approval. Hence, approving.
John Nguyen (johnguy0)
left a comment
There was a problem hiding this comment.
Round-9 review on this commit (59020e2e, defusedxml at module level).
The intent is right and the swap is mostly mechanical. One real gap: the existing except ET.ParseError: handler at workday.py:511 doesn't catch DefusedXmlException, so a hostile Workday payload now crashes FlightCheck instead of falling through to the structured "unparseable XML" CheckResult. Inline comment with two fix options.
requirements.txt pin (defusedxml>=0.7.1,<0.8) looks fine.
Three earlier threads from Graham McMynn (@GrahamMcMynn) on the FlightCheck Graph + PP Admin clients are still unresolved (HIGH on HTTPS validation in derive_environment_id, two MEDIUMs on error_description echo and missing retry adapter). Those are still blocking from my side too; the new commit doesn't touch them.
* HIGH (Graham, pp_admin_client.py:207): derive_environment_id now refuses to attach the Dataverse bearer to a non-HTTPS env_url. Mirrors the _validate_https_url gate in auth.py. * MEDIUM (Graham, graph_client.py:79 + pp_admin_client.py:71): scrub error_description from MSAL failure paths to match the CWE-209 stance taken in auth.py. Echoes 'error' code only. * MEDIUM (Graham, graph_client.py:116 + pp_admin_client.py:_get/_get_all): port the auth.py retry adapter (Retry + HTTPAdapter, GET/HEAD/OPTIONS only, 429/5xx with respect_retry_after_header) to both FlightCheck clients via module-level _SESSION. * MEDIUM (johnguy0/agent, workday.py:511): broaden the post-parse except clause to catch defusedxml.common.DefusedXmlException so attack-path payloads (EntitiesForbidden, ExternalReferenceForbidden, DTDForbidden, NotSupportedError) fall through to the structured 'unparseable XML' result instead of surfacing as an unhandled traceback.
|
Round-8 follow-up: addressed the four remaining open threads.
Pushed as |
|
Round-8 follow-up: addressed the four remaining open threads.
Pushed as |
Round-8 commit 97e9708 added the except clause for DefusedXmlException but the matching import edit silently failed locally, so the previous push left the symbol referenced but unbound. ruff caught it as F821 and CI failed. Restoring the import.
* PR #9: Reference documentation Adds the ESS reference documentation under solutions/ess-agent-kit/src/reference/ess-docs/: - customization/ - agent customization patterns - deployment/ - deployment guidance - flightcheck/ - FlightCheck usage docs - integrations/ - ServiceNow, Workday integration guides - operations/ - operational guidance 46 markdown files. Pulled together from official Microsoft documentation; review for accuracy and any internal-only references. Reviewer: @CavillMason Tracker: #10 * Rename folder solutions/ess-agent-kit -> solutions/ess-maker-skills rename solutions/ess-agent-kit -> solutions/ess-maker-skills * Slice PR #9: add snapshot README explaining why ess-docs/ is vendored Adds src/reference/ess-docs/README.md documenting the snapshot policy: why the docs are vendored (Copilot Chat grounding), source URL, snapshot date (2026-04-29), refresh policy (hand-PR; the auto-sync script was removed in slice PR #3 for corp-NAT rate-limit + silent-overwrite reasons), and explicit non-goals (not a fork, not the place for kit-specific guidance). Addresses the 'feels wasteful to be cloning those' concern raised by @johnguy0 in the PR #29 review thread. The duplication is the explicit tradeoff that gives the kit its domain-correct first-turn output quality; the snapshot stamp makes the staleness contract explicit. Companion: src/examples/ess-samples/ gets an analogous README in slice PR #10.
* PR #9: Reference documentation Adds the ESS reference documentation under solutions/ess-agent-kit/src/reference/ess-docs/: - customization/ - agent customization patterns - deployment/ - deployment guidance - flightcheck/ - FlightCheck usage docs - integrations/ - ServiceNow, Workday integration guides - operations/ - operational guidance 46 markdown files. Pulled together from official Microsoft documentation; review for accuracy and any internal-only references. Reviewer: @CavillMason Tracker: #10 * Rename folder solutions/ess-agent-kit -> solutions/ess-maker-skills rename solutions/ess-agent-kit -> solutions/ess-maker-skills * Slice PR #9: add snapshot README explaining why ess-docs/ is vendored Adds src/reference/ess-docs/README.md documenting the snapshot policy: why the docs are vendored (Copilot Chat grounding), source URL, snapshot date (2026-04-29), refresh policy (hand-PR; the auto-sync script was removed in slice PR #3 for corp-NAT rate-limit + silent-overwrite reasons), and explicit non-goals (not a fork, not the place for kit-specific guidance). Addresses the 'feels wasteful to be cloning those' concern raised by @johnguy0 in the PR #29 review thread. The duplication is the explicit tradeoff that gives the kit its domain-correct first-turn output quality; the snapshot stamp makes the staleness contract explicit. Companion: src/examples/ess-samples/ gets an analogous README in slice PR #10. * Round-2 review on PR #55: address all 6 of John's threads on reference docs * install.md:23 - 'elipsis' -> 'ellipsis'. * install.md:25-28 - Display name / Name / Publisher / Version now have one-line definitions and link out to Power Platform create-solution doc. First-time Makers can no longer complete this checklist incorrectly without realizing it. * install.md:43 - dropped 'can be exported and imported' overstatement; instead links to deploy-overview-alm.md for the actual export/import flow. * prerequisites.md:11+14 - broken Learn-relative cross-link rewritten to absolute https://learn.microsoft.com/microsoft-365-copilot/microsoft-365-copilot-licensing URL on both rows. No more 404 from the vendored snapshot. * deployment-checklist.md:91 - section header sentence-cased: 'Knowledge source documentation' (was 'Knowledge source Documentation'). Now consistent with 'Branding documentation' (line 71) and 'Customizing topics documentation' (line 109). * deployment-checklist.md:95 - dropped the install.md self-link table that promised knowledge-source documentation it didn't deliver. Replaced with concrete in-product steps (Knowledge -> + Add knowledge -> source type) and a real link to the Copilot Studio knowledge-sources documentation.
Adds the Python tooling under
solutions/ess-maker-skills/scripts/:auth.py— MSAL interactive auth + Dataverse REST helperssetup.py— one-shot/setupextraction (components, template configs, workflows, evaluations) → local files +.baseline/+.component-map.jsonpush.py— diff working files vs.baseline/, push CRUD to Dataverseextract.py— bulk extraction helperflightcheck/— pre-deployment readiness validation (Graph + PP Admin clients, 41+ checks)Round 2 review applied (commit
e5dd667a):All 4 CRITICAL findings from PR #13 fixed:
push.pypartial-failure baseline correctly gated (errors == 0 and success > 0)auth.pytoken cache usesos.openwith explicit0o600modeauth.pycache directory chmodded to0o700Most HIGH findings fixed:
--force-deleteflag with interactivetype 'delete' to confirm_call_with_refresh)verify=Trueexplicit, OData URL-encoding, atomic config write, error-message scrubbingRound 3 review applied (this commit):
update_record(will re-add when push.py captures@odata.etag)requirements.txtwith pinned msal/requests--refreshconfigVersion: 1field added to.local/config.jsonfor future schema migrationurllib3.util.retry.RetryTracker: #10