ci: lint only the Python paths that exist on the branch (unblocks PRs #29-#31) - #47
Merged
Conversation
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.
This was referenced May 6, 2026
Merged
John Nguyen (johnguy0)
left a comment
Collaborator
There was a problem hiding this comment.
Small diff doing the right thing. The discover-then-feed shape is correct and the empty-candidates path is handled cleanly, so this should unblock #29/#30/#31 once it lands.
Two things worth flagging. The first is a follow-up worth filing rather than a blocker on this PR; the second is a small content regression in the skip-path notice.
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.
Collaborator
Author
|
John Nguyen (@johnguy0) - both findings fixed in
|
Collaborator
Author
|
John Nguyen (@johnguy0) - both of your findings are addressed and the threads are resolved. PR is green, no other reviewers requested. Mind clicking Approve so we can land this and unblock CI on #29 / #30 / #31? |
saengland
requested review from
Graham McMynn (GrahamMcMynn) and
Srikanth Deshpande (srideshpande)
May 6, 2026 19:12
Srikanth Deshpande (srideshpande)
approved these changes
May 6, 2026
saengland
added a commit
that referenced
this pull request
May 6, 2026
saengland
added a commit
that referenced
this pull request
May 6, 2026
saengland
added a commit
that referenced
this pull request
May 6, 2026
saengland
added a commit
that referenced
this pull request
May 6, 2026
* PR #4: Workday MCP server Adds the Workday MCP server under solutions/ess-agent-kit/src/mcp/workday/. This is a local stdio MCP server that proxies SOAP calls to the customer's Workday tenant on their behalf. SFI focus areas for this review: - SOAP envelope construction (no string concatenation of user input) - Auth header / credential handling (ISU credentials never logged) - Network egress (only customer-provided Workday endpoint, no Microsoft endpoints) - Error response handling (no leakage of customer data in errors) Reviewer: @GrahamMcMynn Tracker: #10 * PR #14: Address review - XML escape, removesuffix, json error handling Three fixes from automated security review: - client.py: escape LLM-supplied params and credentials before SOAP XML interpolation (xml.sax.saxutils.escape) to prevent malformed envelopes and XML injection. Affects _wrap_soap_envelope, get_workers, get_absence_balance, enter_time_off, get_organization. - client.py: replace rstrip('/text()') with removesuffix('/text()'). rstrip strips any character in the set, not the substring; an XPath ending in 'e' would have the 'e' lost. - server.py: wrap json.loads in run_report and extract_from_xml with a _parse_json helper that raises a friendly ValueError instead of leaking JSONDecodeError tracebacks to the LLM. * Rename folder solutions/ess-agent-kit -> solutions/ess-maker-skills rename solutions/ess-agent-kit -> solutions/ess-maker-skills * Address PR #14 review (johnguy0): defusedxml, PII strip, HTTPS, allowlist, write-confirm requirements.txt: - Add defusedxml>=0.7.1 dependency client.py: - Switch untrusted-XML parsing to defusedxml.ElementTree (DET) so the parser is protected against billion-laughs / quadratic-blowup entity expansion. Stdlib ElementTree (ET) kept only for namespace registration and building (no parse calls). - HTTPS enforcement on WORKDAY_BASE_URL - reject http:// to keep ISU credentials out of cleartext. - Silence httpx and httpcore loggers at WARNING so a downstream operator enabling global DEBUG cannot accidentally echo Authorization headers. - __repr__ override hides _password from tracebacks. - httpx clients now opt-out of follow_redirects so a 302 cannot replay the Authorization header to an attacker-controlled host. - Strip SOAP fault <detail> from exceptions - it routinely contains customer PII (employee IDs, names, validation messages) that would flow into the LLM context. Faultstring + status code only; full body logged at DEBUG for the operator. - Strip generic non-SOAP error bodies the same way. - Add jitter to retry backoff (random.uniform(0, 1)) to avoid thundering herd on 429 / network errors. server.py: - Define _READONLY_SOAP_SERVICES allowlist (Human_Resources, Absence_Management, Compensation, Staffing, Talent, Talent_Management, Performance_Management, Benefits_Administration, Payroll, Integrations, Recruiting). call_soap_api and extract_from_xml now reject any service_name outside the set so prompt injection cannot drive destructive Workday operations (Terminate_Employee, Change_Compensation, Cancel_Direct_Deposit, etc.) through the raw-SOAP escape hatch. - request_time_off now requires confirm=True. Without it the tool returns a preview the LLM must show to the user; mutation only happens after explicit user confirmation re-invokes the tool. - Switch ET parsing import to defusedxml. Already fixed in previous commit beaee39: - SOAP XML escaping for credentials (xml.sax.saxutils.escape) - rstrip('/text()') -> removesuffix('/text()') - _parse_json helper for JSON tool args * Address ruff F401 lint findings (slice PR #4) src/mcp/workday/client.py:7-8 - F401 x2: drop unused 'import base64' and 'import json'. Verified neither symbol is referenced anywhere in the file. The XML auth path uses xml_escape (in-line below), not base64; the JSON path uses httpx response.json() builtin, not the stdlib module. Mechanical, behavior-preserving. * Rebrand: 'ESS Copilot Kit' -> 'ESS Maker Kit' in workday MCP server.py (slice PR #4) Inside-repo rename per the naming policy locked in chore PR. Single header docstring update; no code change. * ci: trigger CI re-run with updated workflow from main2 (PR #47)
saengland
added a commit
that referenced
this pull request
May 7, 2026
* PR #5: ServiceNow MCP server Adds the ServiceNow MCP server under solutions/ess-agent-kit/src/mcp/servicenow/. Local stdio MCP server that proxies REST API calls to the customer's ServiceNow tenant. SFI focus areas for this review: - OAuth2 token handling (refresh, expiry, storage) - REST request construction and URL handling - Network egress (only customer-provided ServiceNow endpoint) - Error/response handling (no leakage of customer ticket data) Reviewer: @GrahamMcMynn Tracker: #10 * PR #15: Address review - 429 last_error, json error handling, call_api lockdown, OAuth/OIDC secret env-var indirection * Rename folder solutions/ess-agent-kit -> solutions/ess-maker-skills rename solutions/ess-agent-kit -> solutions/ess-maker-skills * Address PR #15 review (johnguy0): encoded-query injection fix, path traversal validation, denylist server.py: - Added _q() helper that escapes ServiceNow encoded-query special characters (^, null, CR, LF). The encoded-query syntax uses ^ as AND and ^OR as OR; embedding either inside a value lets prompt-injection break out of a single filter clause and append additional ones (e.g. query="x^OR1=1" returns all records; query="x^ORassigned_to=admin" leaks records assigned to a different user). - Wired _q() through every parts.append site in: search_incidents (5 fields), search_hr_cases (4), browse_service_catalog (2), search_cmdb_items (2), resolve_user (4), search_interactions (3). Total: 20 unsafe interpolations closed. client.py: - HTTPS enforcement on SERVICENOW_INSTANCE_URL - reject http:// to keep basic-auth credentials out of cleartext. - Validate `table` and `sys_id` URL path components against strict regexes (_TABLE_NAME_RE = ^[a-z][a-z0-9_]{0,63}$; _SYS_ID_RE = ^[0-9a-fA-F]{32}$). Combined with the protected-table denylist below, prevents path traversal attacks like sys_id="abc/sys_user/admin-id" hitting a different endpoint. - Denylist of protected tables for mutating CRUD: sys_user, sys_user_group, sys_user_role, sys_audit, sys_audit_delete, sys_log, sys_log_transaction, sys_security_acl, sys_properties, oauth_entity, oauth_entity_profile, sys_certificate. delete_record/update_record/create_record on these now raise ValueError. Read-only query_table/get_record/get_stats can still reach them (otherwise we couldn't look up users for resolve_user). - Silence httpx + httpcore loggers at WARNING (DEBUG would echo Authorization headers). - __repr__ override hides _password from tracebacks. - httpx clients now follow_redirects=False (302 cannot replay Authorization to attacker-controlled host). - Add jitter to retry backoff (random.uniform(0,1)) on both 429 and RequestError paths. - Strip response body from HTTPStatusError exceptions; log full body at DEBUG (operator-only) instead of bubbling potentially-PII content into the LLM context. Already addressed in commit 831d343 (verified on this branch): - 429 last_error tracking on max-retry exhaustion - _parse_json helper for JSON tool args - call_api lockdown to method+path allowlist - OAuth/OIDC client_secret env-var indirection so secrets are never passed as tool args Repo hygiene: - Added .gitignore for __pycache__/ and *.pyc in servicenow/. Acknowledged but not in this commit (separate follow-ups): - Refactor admin tools (register_oauth_application, register_oidc_provider, set_system_property, call_api) out of the LLM tool surface entirely and into explicit `/connect` skill scripts. They are still exposed here as MCP tools but call_api is path-allowlisted and OAuth/OIDC use env-var secret indirection. Need a follow-up commit to fully extract. * Address PR #15 review (johnguy0): admin tools off by default + table-API table denylist John's review explicitly rejected the env-var-secret-indirection-only approach for the 3 admin tools. The right end state is to extract them into the /connect skill (PR #6) as scripts the human runs explicitly. As an interim guard until that refactor lands: Admin-tool feature flag (off by default): - Added _ADMIN_TOOLS_ENABLED reading SERVICENOW_MCP_ENABLE_ADMIN_TOOLS env var (1/true/yes/on enables, anything else disables). - New _require_admin_tools(tool_name) raises PermissionError with a clear message pointing at /connect and the env-var unblock. - Wired the guard into: register_oauth_application register_oidc_provider set_system_property - Updated docstrings on each to label them ADMIN TOOL with the unblock procedure. call_api: data-permissive table allowlist (HIGH per John's PR #15 discussion_r3174960930): - Method+path allowlist already blocked admin endpoint paths but permitted non-GET on /api/now/table/* unconditionally - so a prompt-injected LLM could still DELETE /api/now/table/sys_user/<admin> PATCH /api/now/table/sys_properties/<security-prop> POST /api/now/table/sys_user_grmember (privilege escalation) - Added _CALL_API_TABLE_DENYLIST_NON_GET covering sys_user / role / group / has_role, sys_properties, sys_audit, sys_audit_delete, sys_security_acl, sys_security_diag, oauth_entity, oauth_entity_profile, sys_oidc_provider, sys_certificate, sys_script*, sys_ws_operation. - _CALL_API_TABLE_PATH_RE parses the table name out of the path; on non-GET methods, denied tables raise PermissionError. GET (read) is permitted because the typed tools (resolve_user, etc.) need to query sys_user records. Follow-up (separate PR after merge): full extraction of admin tools into solutions/ess-maker-skills/scripts/connect_servicenow.py invoked from the /connect skill, removing them from server.py entirely. * Address ruff E402 lint finding (slice PR #5) src/mcp/servicenow/server.py:166 - E402: 'import re as _re' was placed after non-import statements (the EXCLUDED_TABLES set definition). Moved to the top with the other stdlib imports per PEP 8. The compiled regex constant _CALL_API_TABLE_PATH_RE stays where it is (late module-level definition is fine; only the import had to move). Mechanical, behavior-preserving. * Round-1 review: address johnguy0 architect/security/perf/prompt findings (slice PR #5) ARCHITECT * Promote client._request to public client.request; _request kept as backward-compat alias. call_api in server.py now routes through client.request (no longer reaches into a private method). * Consolidate the table denylist into client.EXCLUDED_TABLES and import from server.py. The two parallel lists were already drifting (sys_user_grmember, sys_user_has_role, sys_oidc_provider, sys_security_diag, sys_script* were only on the server side; sys_log*, sys_log_transaction, oauth_entity_profile, sys_certificate were only on the client side). Single source now; the server-side _CALL_API_TABLE_DENYLIST_NON_GET is the import alias. SECURITY * Encoded-query injection: list_oauth_applications:641, list_oidc_providers:721, check_plugin_active:808, get_system_properties:824 were interpolating the LLM-supplied value raw into sysparm_query. Patched all four to route through _q(). Sample: 'name^OR1=1' would have recreated the encoded-query injection that _q() exists to prevent. * log_copilot_summary at server.py:621: data.update(extra) let LLM-supplied additional_fields clobber the tool-controlled u_interaction / u_summary / u_resolution. Reversed merge order to data = {**extra, **data} so tool-controlled fields always win. Added isinstance(extra, dict) guard so a JSON array/scalar surfaces a clear error rather than crashing the merge. * client.py logger.debug body emission removed. The comment above the exception promised the response body would not surface (PII / internal field metadata). The debug line undermined that promise: any operator who flipped the global log level to DEBUG would have leaked the body. If an operator needs the body for triage they should reproduce with httpx logging enabled explicitly. PERFORMANCE * Long-lived httpx.AsyncClient cached on self via _ensure_client (lazy init, asyncio.Lock around the create), closed on aclose(). Saves the 50-200ms TLS handshake per call across the typical 5-50 calls in an LLM session. * Retry path extended to 502/503/504 with the same backoff used for 429 (one transient gateway error during a deploy or restart no longer fails on the first try). * _parse_retry_after handles RFC 9110 HTTP-date format, falls back to delta-seconds, falls back to None on parse failure (the call site then uses 2**attempt). Previously int(retry_after) would raise on a date value. PROMPT-ENGINEERING * FastMCP server-level instructions now include a 'prefer typed tools' routing line. This reaches the LLM before any individual tool docstring; highest-leverage place to put routing. * query_table, create_record, update_record, delete_record docstrings each carry a one-sentence routing line pointing the LLM at the typed alternative. * search_incidents:assigned_to docstring fixed: ServiceNow matches sys_id only on assigned_to, so 'display name or sys_id' silently returned zero results when a name was passed. Doc now tells the LLM to call resolve_user first if it only has a name. * log_copilot_summary:additional_fields docstring lists the supported u_* columns (u_channel, u_outcome, u_session_id, u_disposition), states the JSON-object requirement, and warns that unknown columns surface as ServiceNow validation errors. All edits behavior-preserving for the supported call paths; tightening on the abuse paths. Lint clean (ruff). * Drop accidentally-committed __pycache__ from prior commit The previous commit picked up scripts/__pycache__/ from a local syntax check (python -m py_compile). They never should have been tracked. The repo .gitignore already excludes __pycache__/ globally; these slipped in via 'git add -A'. Remove + future commits will respect .gitignore. * ci: trigger CI re-run with updated workflow from main2 (PR #47)
saengland
added a commit
that referenced
this pull request
May 7, 2026
* PR #3: Python scripts 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 * PR #13: Address review - escape XML special chars in SOAP credentials 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 folder solutions/ess-agent-kit -> solutions/ess-maker-skills rename solutions/ess-agent-kit -> solutions/ess-maker-skills * Address PR #13 review (johnguy0): P0 push.py + auth.py fixes, my/ -> .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 * Address PR #13 CodeQL alert + setup.py:38 dead-code line 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). * workday.py: split _resolve_workday_creds to break CodeQL taint chain 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. * workday.py: drop tenant from print() calls (CodeQL clear-text logging 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. * Address ruff lint findings (slice PR #3) 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). * Round-3 review: address remaining johnguy0 findings (slice PR #3) 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). * Round-4 review: address johnguy0 CRITICAL + HIGH defects (slice PR #3) 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. * Drop accidentally-committed __pycache__ from prior commit 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. * Round-5 review: eval-orphan fail-closed, atomic gate, rename staging, 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. * Rebrand: 'ESS Copilot Kit' -> 'ESS Maker Kit' in scripts/ headers + strings (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. * Round-6 review: fix workflow-rename keying, atomic two-phase persist, 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. * Round-7 review: clean up partial phase-3 .tmp leftovers (slice PR #3) Addresses johnguy0 round-6 LOW (push.py:929). Phase-3 os.replace is atomic per-file but loops across meta files. If a later rename fails, earlier metas committed but the remaining *.tmp siblings stayed on disk - /setup --refresh fixed the baseline but the next /push would re-encounter the cruft. Track committed count and clean up only the uncommitted leftovers in the except branch (mirrors the phase-2 cleanup pattern). * ci: trigger CI re-run with updated workflow from main2 (PR #47) * PR #29 round-7 follow-ups: address the 3 remaining @johnguy0 threads. #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). * PR #29: harden Workday XML parsing (defusedxml at module level) 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. * Round-8 review: address remaining 4 threads on PR #29 * 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. * fix(workday): import DefusedXmlException (round-8 follow-up; ruff F821) 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The current
Lint Pythonjob in.github/workflows/ci.ymlhard-codes bothsolutions/ess-maker-skills/scripts/ANDsolutions/ess-maker-skills/src/mcp/as args to ruff andcompileall. Ruff returnsE902 No such file or directorywhenever either path is absent, which is the case on every per-PR review branch:scripts/onlyE902onsrc/mcp)scripts/+src/mcp/workday/scripts/+src/mcp/servicenow/This is a workflow bug, not a code bug on any of those PRs.
Change
Replace the binary
has_pythongate with adiscoverstep that walks each candidate path, adds it to the output ONLY if it exists AND contains.pyfiles, and feeds the resolved list to ruff andcompileall.scripts/andsrc/mcp/remain the only candidate directories - this PR doesn't expand the lint surface, it just stops hard-coding paths that aren't on the branch.Verification
has_python=falseand skip).Follow-up
Once main2 has this fix, I'll merge it into the three review branches and confirm
Lint Pythonflips to SUCCESS on each.