diff --git a/.agents/skills.json b/.agents/skills.json index 3819e36ed8..dc1704ce91 100644 --- a/.agents/skills.json +++ b/.agents/skills.json @@ -3,6 +3,9 @@ { "path": "agents/skills/dc-import-diagnostics" }, + { + "path": "agents/skills/dc-import-code-review" + }, { "path": "agents/skills/dc-import-postmortem-doc" } diff --git a/agents/README.md b/agents/README.md index 6e554d21a8..d99ccecd09 100644 --- a/agents/README.md +++ b/agents/README.md @@ -6,6 +6,10 @@ configuration, and support scripts. For local tools, Python dependencies, Google Cloud authentication, and the optional sibling checkout, see [dependency setup](dependency-setup.md). +For authoring conventions, see [agent skill authoring](docs/skill-authoring.md). +For diagnostics-specific structure, see +[DC import diagnostics authoring](docs/dc-import-diagnostics-authoring.md). + ## Inspect or diagnose imports For read-only ET import information or diagnosis, use the @@ -13,3 +17,42 @@ For read-only ET import information or diagnosis, use the Copy the prompt into the agent conversation and append the specific import question. The prompt routes the request through the repository-owned `dc-import-diagnostics` skill and its bounded operational references. + +## Review import changes + +For a read-only review of staged, unstaged, branch-comparison, or pull request +changes under `scripts/**` and `statvar_imports/**`, use the +[`dc-import-code-review` starter prompt](prompts/dc-import-code-review-starter.md). +The skill returns P0-P3 findings, meaningful positive findings, coverage, and +verification. + +## Mine import review signals + +To collect positive and corrective review signals from merged import pull +requests, use the `dc-import-review-signal-miner` skill through its +[`starter prompt`](prompts/dc-import-review-signal-miner-starter.md). The skill +produces a complete comment audit and a projection containing only strong, +unambiguous signals. It does not update guidelines or create a pull request. + +Before running the prompt, ensure the agent has network access to GitHub and +can write to a temporary directory and the selected output directory. Then run +the skill's +[`prerequisite checker`](skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh): + +```bash +bash agents/skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh +``` + +The checker validates `git`, GitHub CLI, authentication, and the exact `gh` +capabilities used by the skill. The mining skill does not require standalone +`jq`, Python, Google Cloud CLI, GCS access, or write access to the Data Commons +repository. Its GitHub commands were tested with GitHub CLI 2.74.2; the checker +reports the installed version without treating 2.74.2 as a minimum. + +## Merge import review signals + +To merge considered signals into the import code-review guidelines, use the +[`import review signal merge prompt`](prompts/import-review-signal-merge.md). +Provide the miner output directory and the data repository path. The prompt +requires a clean current branch, leaves guideline edits uncommitted, and writes +a decision report beside the miner output. diff --git a/agents/common/scripts/skill_contract_test.py b/agents/common/scripts/skill_contract_test.py index cf2166dbfb..5bcc48c5e3 100644 --- a/agents/common/scripts/skill_contract_test.py +++ b/agents/common/scripts/skill_contract_test.py @@ -258,11 +258,7 @@ def test_registry_points_to_versioned_skill(self): self.assertTrue((self._repo_root / path / 'SKILL.md').is_file()) def test_reachable_agent_markdown_links_resolve(self): - registry = json.loads(self._read('.agents/skills.json')) - entrypoints = [ - self._repo_root / entry['path'] / 'SKILL.md' - for entry in registry['entries'] - ] + entrypoints = sorted((self._agents_root / 'skills').glob('*/SKILL.md')) entrypoints.append(self._agents_root / 'README.md') errors = _local_markdown_link_errors(self._repo_root, entrypoints) @@ -428,5 +424,76 @@ def test_runtime_environment_registry_is_minimal_and_complete(self): self.assertTrue(environment[section][field]) +class ImportCodeReviewSkillContractTest(unittest.TestCase): + + def setUp(self): + self._repo_root = Path(__file__).parents[3] + self._agents_root = self._repo_root / 'agents' + self._skill_root = (self._agents_root / 'skills/dc-import-code-review') + self._skill_path = self._skill_root / 'SKILL.md' + self._prompt_path = (self._agents_root / + 'prompts/dc-import-code-review-starter.md') + + def _read(self, relative_path: str) -> str: + return (self._repo_root / relative_path).read_text(encoding='utf-8') + + def test_review_skill_is_registered_and_discoverable(self): + registry = json.loads(self._read('.agents/skills.json')) + paths = [entry['path'] for entry in registry['entries']] + readme = self._read('agents/README.md') + prompt = self._prompt_path.read_text(encoding='utf-8') + + self.assertIn('agents/skills/dc-import-code-review', paths) + self.assertIn('prompts/dc-import-code-review-starter.md', readme) + self.assertIn('`dc-import-code-review` skill', prompt) + + def test_review_skill_keeps_core_contract(self): + skill = self._skill_path.read_text(encoding='utf-8') + + for marker in ('scripts/**', 'statvar_imports/**', 'read-only', 'P0', + 'P1', 'P2', 'P3', 'Finding', 'Impact', 'Recommendation'): + with self.subTest(marker=marker): + self.assertIn(marker, skill) + + links = {path for path, _, _ in _local_markdown_links(skill)} + self.assertTrue({ + 'references/guidelines.md', + '../../common/references/import-automation/manifest.md', + }.issubset(links)) + + def test_review_guidance_stays_single_and_lightweight(self): + references = sorted(path.name for path in (self._skill_root / + 'references').glob('*.md')) + guidelines = (self._skill_root / + 'references/guidelines.md').read_text(encoding='utf-8') + + self.assertEqual(['guidelines.md'], references) + self.assertFalse((self._skill_root / 'README.md').exists()) + self.assertFalse((self._skill_root / 'scripts').exists()) + + for stale_content in ('DCIR-', 'Evidence:', 'Last verified:', + '["support@datacommons.org"]', 'logging.fatal()', + 'exit(1)'): + with self.subTest(stale_content=stale_content): + self.assertNotIn(stale_content, guidelines) + + for validation_reference in ( + '../../../../tools/import_validation/README.md', + '../../../../tools/import_validation/Validations.md'): + with self.subTest(validation_reference=validation_reference): + self.assertIn(validation_reference, guidelines) + + def test_review_guidance_uses_only_relative_paths(self): + sources = [self._skill_path, self._prompt_path] + sources.extend((self._skill_root / 'references').glob('*.md')) + + for source in sources: + text = source.read_text(encoding='utf-8') + with self.subTest(source=source.name): + self.assertNotIn('/Users/', text) + self.assertNotIn('file://', text) + self.assertNotIn('/', text) + + if __name__ == '__main__': unittest.main() diff --git a/agents/docs/dc-import-diagnostics-authoring.md b/agents/docs/dc-import-diagnostics-authoring.md new file mode 100644 index 0000000000..22ed8d8f4e --- /dev/null +++ b/agents/docs/dc-import-diagnostics-authoring.md @@ -0,0 +1,95 @@ +# DC import diagnostics authoring + +Use this guide when extending +[dc-import-diagnostics](../skills/dc-import-diagnostics/SKILL.md) or its +troubleshooting guidance. + +## Preserve the routing boundary + +- Factual inspection uses the operation routes in `SKILL.md`. +- Failed, stalled, or unexpected-output requests load the + [troubleshooting entry point](../skills/dc-import-diagnostics/troubleshooting/troubleshooting.md). +- A named failure domain or suspected cause routes directly to its guide. +- An unknown scenario gathers only enough evidence to identify the failure + domain. + +Keep `SKILL.md` responsible for global scope, safety, and information routes. +Keep `troubleshooting.md` responsible for domain selection and the bounded +fallback. + +| User request | Route | +|---|---| +| "Show the current ImportStatus." | Use the factual route in `SKILL.md`. | +| "Why did this import fail?" | Open the troubleshooting entry point. | +| "Check whether this Batch job ran out of memory." | Open the Batch runtime guide and test that hypothesis first. | + +## Keep diagnosis hypothesis-driven + +### Investigation loop + +```text +triage +→ identify the failure domain +→ choose a plausible hypothesis +→ verify or refute + ├─ confirmed → state the likely cause → mitigate or fix + ├─ refuted → test the next plausible hypothesis + └─ unknown → report missing evidence or the next useful check +``` + +This approach follows the iterative hypothesis-testing model in Google SRE's +[Effective Troubleshooting](https://sre.google/sre-book/effective-troubleshooting/). +This guide remains the source of truth for repository-specific structure. + +### Apply a hypothesis + +For each hypothesis, keep together: + +- when to consider it; +- evidence that confirms or refutes it; and +- mitigation when confirmed. + +During investigation: + +- A user-supplied hypothesis changes investigation order, not the evidence + needed to confirm it. +- Treat indirect signals as clues. +- Treat unavailable evidence as unknown, not refuted. +- Test only plausible hypotheses. Do not gather all evidence up front. + +Example: + +1. A failed Batch job routes to the Batch runtime guide. +2. If out of memory is plausible, follow + [Out of memory](../skills/dc-import-diagnostics/troubleshooting/batch-runtime.md#out-of-memory) + to verify or refute it. +3. If confirmed, use that guide's mitigation. +4. If refuted, test the next plausible runtime hypothesis. + +### Separate diagnosis from evidence collection + +| Location | Owns | +|---|---| +| Domain guide | Hypotheses, evidence interpretation, and mitigation | +| Operational reference, such as [Cloud Batch operations](../skills/dc-import-diagnostics/references/batch.md) | Commands, identifiers, bounds, and evidence-retrieval failure handling | + +- Add a new evidence operation to its operational reference, then link the + hypothesis to it. +- For example, a troubleshooting guide may request bounded Batch logs. Keep + the command, filters, and bounds in `batch.md`. +- Do not repeat skill-wide safety or remediation policy in each guide. +- Do not impose a fixed playbook schema. Use the smallest structure that makes + the issue clear. + +## Add a troubleshooting guide + +1. Add one guide for a coherent failure domain or related set of issues. +2. Link it from `troubleshooting/troubleshooting.md` using the symptom language + users will provide. +3. Link its evidence steps to the relevant operational reference sections. +4. Add representative cases to the + [diagnostics golden queries](../evals/dc-import-diagnostics.md). + +The contract tests ensure every troubleshooting guide is reachable from the +entry point and every referenced file or section exists. Do not add schemas, +IDs, templates, or exhaustive prose tests without a demonstrated need. diff --git a/agents/docs/skill-authoring.md b/agents/docs/skill-authoring.md new file mode 100644 index 0000000000..260c2bce4a --- /dev/null +++ b/agents/docs/skill-authoring.md @@ -0,0 +1,91 @@ +# Repository agent skill authoring + +Use this guide when adding or reorganizing skills under `agents/`. + +## Repository locations + +| Content | Location | +|---|---| +| Runtime skill entry point | `agents/skills//SKILL.md` | +| Skill-specific references | `agents/skills//references/` | +| Shared agent-readable references | `agents/common/references/` | +| Shared configuration | `agents/common/config/` | +| Shared Python helpers and tests | `agents/common/scripts/` | +| Python helper wrapper | `agents/common/run_python.sh` | +| Starter prompts | `agents/prompts/` | +| Human maintenance guidance | `agents/docs/` | +| Golden evaluation queries | `agents/evals/` | + +Keep contributor guidance in `agents/docs/`, outside runtime skill +directories. + +## Register and expose skills + +- Register each skill's canonical directory path in `.agents/skills.json`. +- Add human entry points, starter prompts, and authoring guides to + `agents/README.md` when they need to be discoverable. +- Remove obsolete names instead of adding aliases unless compatibility is + explicitly required. + +## Keep ownership clear + +- Keep instructions and operations used by one skill inside that skill. +- Move content to `agents/common/` only when it has another real consumer. +- Keep common references consumer-neutral. Skills may link to common + references; common references must not link into skill directories. +- Keep shared configuration and reusable Python execution helpers in + `agents/common/`. +- Prefer a service or domain reference over a separate file for every command. + Do not introduce a recipe hierarchy unless a concrete need emerges. + +## Keep runtime guidance focused + +- Describe the user problem and the skill's capability, not its file inventory. +- State clear `Use when` and `Do not use for` boundaries. +- Keep scope, safety, and common routes in `SKILL.md`. Link details needed only + in some cases. +- Write routes using terms users will recognize. Clarify ambiguous terms instead + of silently choosing a meaning. +- Use source-relative Markdown links. Referenced sections must have unique, + plain ATX headings; link text does not need to match the heading. + +## Useful authoring tips + +These tips complement the target agent's guidance. Follow client-specific rules +when they differ. + +- Use representative user requests to shape triggers, routes, and tests. For + example, "Why did this import fail?" should route to troubleshooting. +- Focus on repository knowledge, procedures, and non-obvious edge cases. Skip + background the agent already handles well. +- State the situation and action together. For example, "If no Batch job ID + exists, inspect Scheduler." +- Use short sentences and consistent terms. +- Prefer one source for detailed information. Repeat small details when useful. + For example, keep a full Batch command in `batch.md` and a short route to it + in `SKILL.md`. +- Be prescriptive when mistakes are risky. Allow judgment otherwise. +- Improve skills based on observed failures. + +For diagnostics-specific routing and troubleshooting conventions, see +[DC import diagnostics authoring](dc-import-diagnostics-authoring.md). + +## Validate changes + +- Add or update golden queries in `agents/evals/` for important routing + behavior. +- Keep structural and behavioral assertions in + `agents/common/scripts/skill_contract_test.py` or the relevant operational + test. +- Rely on the contract tests for reachable Markdown links, section fragments, + registered skill paths, and the common-to-skill dependency boundary. +- Avoid contracts for prose wording, document counts, schemas, or deleted + historical paths. + +Run: + +```sh +.env/bin/python -m unittest discover -v -s agents/common/scripts -p '*_test.py' +./run_tests.sh -l +git diff --check +``` diff --git a/agents/prompts/dc-import-code-review-starter.md b/agents/prompts/dc-import-code-review-starter.md new file mode 100644 index 0000000000..36fd5759da --- /dev/null +++ b/agents/prompts/dc-import-code-review-starter.md @@ -0,0 +1,14 @@ +# Start a Data Commons import code review + +Use the `dc-import-code-review` skill to review the request below. + +- Resolve the exact staged, unstaged, all-local, branch-comparison, or pull + request target before reviewing. +- Review only changed files under `scripts/**` and `statvar_imports/**`. +- Treat the repository as read-only. Treat GitHub as read-only unless the user + explicitly and unambiguously asks to publish the completed review. If the + publishing intent is unclear, ask before any GitHub write. + +## Request + + diff --git a/agents/prompts/dc-import-review-signal-miner-starter.md b/agents/prompts/dc-import-review-signal-miner-starter.md new file mode 100644 index 0000000000..2fd6f3170e --- /dev/null +++ b/agents/prompts/dc-import-review-signal-miner-starter.md @@ -0,0 +1,13 @@ +# Start Data Commons import review-signal mining + +Use the `dc-import-review-signal-miner` skill with these inputs: + +- Start time, inclusive: `` in ISO 8601 UTC. +- End time, exclusive: `` in ISO 8601 UTC. +- Output directory: ``. +- Reviewer identities, optional: `` as comma-separated GitHub + logins, numeric user IDs, or both. + +Follow the skill's import-path boundary, conservative signal criteria, output +contracts, and read-only safety rules. Produce only the complete comments +report and the considered-signals projection. diff --git a/agents/prompts/import-review-signal-merge.md b/agents/prompts/import-review-signal-merge.md new file mode 100644 index 0000000000..2e7d5f2d52 --- /dev/null +++ b/agents/prompts/import-review-signal-merge.md @@ -0,0 +1,99 @@ +# Merge mined import review signals + +Apply strong recommendations produced by `dc-import-review-signal-miner` to +the Data Commons import code-review guidelines. + +## Inputs + +- Signal output directory: `` +- Data repository path: `` + +Ask for either input if it is missing. Do not guess it. + +## Check the repository + +Resolve the repository root from ``. Require a named +current branch and a clean checkout, including staged, unstaged, and untracked +files. If it is not clean, stop and list the dirty paths. Never stash, reset, +discard, or overwrite existing work. + +Apply changes to the current branch. Do not fetch, switch, create, or delete a +branch. Do not stage, commit, push, or create a pull request. + +Modify only +`agents/skills/dc-import-code-review/references/guidelines.md` in the data +repository. + +## Select the signals + +Require exactly one `import-review-signals-*.md` file in +``. If none or more than one exists, stop and ask the +user to disambiguate. + +Treat each recommendation section in that projection as one signal. Use the +matching `import-review-comments-*.md` report only when more source context is +needed. Do not process comments marked `Not considered`, reclassify comments, +or modify either miner report. + +## Merge the signals + +Compare each signal semantically with the existing guidelines and current +repository implementation. Assign one disposition: + +- `Added`: The recommendation is strong, general, new, and non-conflicting. + Add one concise, neutral recommendation bullet under the best existing + heading. Add a heading only when no existing heading fits. +- `Already covered`: An existing guideline expresses the same desired + behavior. Do not replace, rewrite, or duplicate it. +- `Conflict`: The recommendation contradicts existing guidance or current + implementation. Do not change the guidelines. +- `Skipped`: The recommendation is ambiguous, too specific, unsupported, or + cannot be merged safely. Do not change the guidelines. + +If several signals support the same new guideline, add it once and report a +decision for every signal. If a signal contains a clearly separable new point, +add only that point. Otherwise prefer no change. + +Keep the guidelines simple: + +- Phrase positive and corrective signals as neutral recommendations. +- Preserve existing recommendations and organization. +- Do not add guideline IDs, severity, confidence, evidence metadata, dates, + reviewer identities, or source links. +- Do not duplicate generic repository language or style guidance. + +## Write the merge report + +Write +`/import-review-signal-merge-.md` +using the current UTC time. Do not overwrite an existing report. + +Include a summary with the projection path, repository root, current branch, +HEAD commit, and disposition counts. Then include every signal using: + +```markdown +### + +- Disposition: Added | Already covered | Conflict | Skipped +- Source comments: +- Existing guidance: | None +- Change: | None +- Rationale: +``` + +End the report with checks run, checks not run, changed repository paths, and +explicit statements that changes were not staged, committed, pushed, or used +to create a pull request. + +## Verify and finish + +- Inspect the final diff and confirm no existing guideline was replaced. +- Confirm the only intended repository edit is `guidelines.md`. If the signal + output directory is inside the repository, treat the merge report as an + output artifact and never stage it. +- Run `git diff --check`. +- Run `python3 -m unittest agents.common.scripts.skill_contract_test` with the + repository's configured Python environment when available. Do not install + dependencies solely for this run; record unavailable checks in the report. +- Leave all changes unstaged and uncommitted on the current branch. +- Return the merge-report path, disposition counts, and verification results. diff --git a/agents/skills/dc-import-code-review/SKILL.md b/agents/skills/dc-import-code-review/SKILL.md new file mode 100644 index 0000000000..c942fbaaea --- /dev/null +++ b/agents/skills/dc-import-code-review/SKILL.md @@ -0,0 +1,275 @@ +--- +name: dc-import-code-review +description: Reviews staged, unstaged, branch-comparison, or GitHub pull request changes to Data Commons imports under scripts/** and statvar_imports/**. Use when an import author or reviewer asks for an import-specific code review. Do not use for unrelated repository code or deployed-import diagnosis. +--- + +# Review Data Commons import changes + +Review one explicitly selected import change set. Inspect every in-scope changed +hunk, report only supported findings, and leave the repository unchanged. Leave +GitHub unchanged unless the user explicitly authorizes publishing under +[Publish an explicitly authorized review](#publish-an-explicitly-authorized-review). + +## Safety and scope + +- Resolve the repository root with `git rev-parse --show-toplevel`. From that + root, verify that `scripts/` and `statvar_imports/` exist, and run Git + operations using repository-relative paths. +- Treat the repository as read-only. Never edit the import, stage files, + discard local changes, or change the active branch. +- Treat GitHub as read-only by default. Publish the completed review only when + the user explicitly and unambiguously asks to post it to the selected pull + request. A request to review a pull request does not authorize publishing. If + publishing intent is unclear, ask before any GitHub write. +- Even when publishing is authorized, never approve or request changes, resolve + review comments, merge or close a pull request, or edit or delete GitHub + content. Publishing is limited to one comment-only review. +- Enumerate every changed path before filtering the review. +- Report findings only for changed files under `scripts/**` and + `statvar_imports/**`. +- List changed paths outside those directories as skipped. Read unchanged or + shared code only when needed to understand an in-scope change, and do not + report unrelated findings from that context. +- Stop and report that there are no import changes when the selected target has + no changed paths in scope. Do not fall back to a general repository review. + +## Resolve the review target + +Require exactly one target before reviewing: + +| User intent | Change set | +|---|---| +| Staged changes | Index versus `HEAD` | +| Unstaged changes | Working tree versus index, plus untracked files | +| All local changes | Working tree and index versus `HEAD`, plus untracked files | +| Changes against a branch | Merge base of the explicit base ref and `HEAD` through `HEAD` | +| Pull request | Exact pull request diff and head commit | + +If the review target is ambiguous, ask whether to review staged, unstaged, all +local, branch-comparison, or pull request changes. If a branch comparison lacks +an exact base ref, ask for it. Do not infer `master`, `main`, a remote, or a +combination of local changes. + +Use Git to acquire local targets without changing repository state: + +- For staged changes, use `git diff --cached`. +- For unstaged tracked changes, use `git diff`. Enumerate untracked files with + `git ls-files --others --exclude-standard` and treat their complete contents + as changed. +- For all local changes, use `git diff HEAD` and include the complete contents + of untracked files. +- For a branch comparison, resolve `git merge-base HEAD` and compare + that commit through `HEAD`. Exclude staged, unstaged, and untracked work + unless the user separately selected local changes. + +For each target, first collect its complete changed-path list without a +pathspec. Then acquire the in-scope diff for `scripts/` and `statvar_imports/`. +Preserve additions, modifications, deletions, and renames in the coverage +record. + +## Review a pull request + +Accept a `datacommonsorg/data` pull request number or URL. Use `gh` for every +GitHub operation. + +Start with these read-only commands: + +```bash +gh pr view --repo datacommonsorg/data \ + --json number,title,url,baseRefName,baseRefOid,headRefOid,changedFiles,additions,deletions,files +gh pr diff --repo datacommonsorg/data +``` + +1. Use the metadata to record the pull request identity, exact base and head + SHAs, changed-file count, additions, and deletions. +2. Use the pull request diff as the authoritative description of the change + set when it is complete. Enumerate its changed files before applying the + import-path filter. +3. Review from the diff when it is complete and contains enough context. +4. Create a detached temporary worktree at the exact head SHA when the diff is + unavailable or incomplete, a changed file is renamed, binary, or generated, + behavior crosses files, complete manifest references are needed, or focused + checks require a checkout. + +Never run `gh pr checkout` over the active worktree. When a temporary worktree +is needed: + +- Create a unique root with `mktemp -d` and record its resolved path. +- Fetch the pull request head and base from `datacommonsorg/data` without + switching the active branch. +- Verify the fetched head and base SHAs match `gh pr view` before adding the + detached worktree. +- Continue to use the pull request diff to identify changed lines when it is + complete. If it is unavailable or incomplete, compare the verified base and + head commits locally and report that fallback as a limitation. +- Run Python checks with the temporary worktree root as the working directory. + Prefer `./run_tests.sh -p `; it creates + and uses that worktree's `.env`. Pass a test directory, not a test file. +- Do not use global Python or an environment from another checkout. If a direct + Python command is necessary, run `./run_tests.sh -r` first and then use + `.env/bin/python`. If setup fails, report the check as not run instead of + falling back to another Python environment. +- Remove the detached worktree with `git worktree remove` after the review. + Remove no path that was not created and validated by this run. + +If GitHub metadata, the diff, or a required fetch is incomplete, report the +limitation instead of claiming complete coverage. + +## Load review guidance + +Read [import code review guidelines](references/guidelines.md) for every +review. Follow its links to repository documentation when relevant to the +changed import files. Apply a recommendation only when it is relevant to the +changed behavior. + +When any in-scope `manifest.json` changes, also read the current shared +[import manifest reference](../../common/references/import-automation/manifest.md). +Use the shared reference for current fields and requirements; do not +reconstruct the manifest contract from historical guidance. + +Apply repository instructions to changed import code without duplicating their +generic language and style rules in the import guidelines. + +If documentation conflicts with the current implementation, treat the code as +the implementation truth. Call out the conflict and its implications in the +review; do not resolve it silently. + +## Review changed behavior + +- Inspect every in-scope changed hunk and enough surrounding context to + understand the resulting behavior. +- Trace changed manifests to referenced scripts and inputs when those + relationships are affected. +- Trace downloads and transformations far enough to evaluate completeness, + failure handling, retries, data loss, mappings, validation, and tests. +- Anchor each finding to a changed line whenever possible. Unchanged context + may support a finding but may not become an unrelated finding. +- Report only issues introduced or exposed by the selected change set. Do not + turn pre-existing problems into review findings. +- Run only focused checks useful for the selected import. State exactly what + ran, what passed or failed, and what could not run. +- Prefer hermetic checks. Do not run tests that call live source, Data Commons, + or cloud APIs or require credentials unless the user explicitly requests + them and the prerequisites are available. Bound each check, and stop and + report a stalled check instead of waiting indefinitely. +- Prefer no finding over a speculative finding. State missing context as a + limitation. +- Report a positive finding only for a meaningful, reusable import practice. + Do not praise routine syntax, formatting, or merely the absence of a defect. + +## Assign remediation priority + +Use priority to describe remediation urgency, not confidence: + +| Priority | Meaning | +|---|---| +| P0 | Immediate security issue, broad data corruption, or production emergency | +| P1 | Likely incorrect data, import failure, or serious reliability problem | +| P2 | Realistic validation, testing, maintainability, or operational risk | +| P3 | Minor, localized improvement | + +Positive findings are unranked. If there are no actionable findings, say so +directly without claiming correctness beyond the reviewed evidence. + +## Report the review + +Use this Markdown structure for the review: + +```markdown +## Review scope + +- Target: +- Reviewed: +- Skipped: + +## Findings + +### [P1] + +- path/to/file.py:42 - Brief description + - Finding: What the changed code does incorrectly. + - Impact: Concrete data, operational, or maintenance consequence. + - Recommendation: Specific corrective action. + +## Positive findings + +- path/to/file.py:80 - Meaningful reusable pattern ✓ + - Finding: Good - What was done correctly. + +## Coverage + +| File | Status | Result | +|---|---|---| +| path/to/file.py | Reviewed | One P1 finding | +| path/to/manifest.json | Reviewed | No findings | + +## Verification and limitations + +- Checks run: +- Checks not run: +- Limitations: +``` + +Order actionable findings by P0 through P3. Every actionable finding must +include `Finding`, `Impact`, and `Recommendation`. Every positive finding must +include `Finding: Good - `. Use the exact `File`, +`Status`, and `Result` coverage columns shown above, and include every in-scope +changed file, including files with no findings. + +## Publish an explicitly authorized review + +Apply this section only to a pull request review. Complete the review before +performing any GitHub write. + +An explicit publishing request in the original request or a later follow-up is +sufficient authorization; do not ask again. If the user asks only for a review, +mentions publishing as an option, or otherwise leaves the action unclear, ask +whether to publish and wait for the answer. + +Immediately before publishing, fetch the pull request's `headRefOid` again with +`gh pr view`. Compare it with the head SHA that was reviewed. If they differ, do +not publish stale findings; report the change and ask whether to review the new +head. + +Prepare one comment-only review: + +- Post actionable findings inline only when they can be anchored to a changed + line in the current pull request diff. +- Put unanchored findings, positive findings, coverage, verification, and + limitations in the review body. Do not post positive findings inline. +- Use repository-relative paths, the verified head SHA as `commit_id`, and + `line` with `side`: `RIGHT` for an added line and `LEFT` for a deleted line. +- Use `event: COMMENT`. Never use `APPROVE` or `REQUEST_CHANGES`. + +Create a single review so its body and inline comments are submitted together: + +```bash +gh api --method POST \ + repos/datacommonsorg/data/pulls//reviews \ + --input \ + --jq '{id, state, html_url, commit_id}' +``` + +Use this payload shape. Omit `comments` when there are no inline findings. + +```json +{ + "commit_id": "", + "event": "COMMENT", + "body": "", + "comments": [ + { + "path": "scripts/source/import/process.py", + "line": 42, + "side": "RIGHT", + "body": "**[P1] Finding title**\n\nFinding: ...\n\nImpact: ...\n\nRecommendation: ..." + } + ] +} +``` + +If the execution environment requires approval for the GitHub write, request +it. If approval is denied or authentication lacks write permission, report that +nothing was published. After a successful response, report the review URL and +the number of inline comments. If the result is uncertain, inspect existing +reviews for the verified head before retrying so the review is not duplicated. diff --git a/agents/skills/dc-import-code-review/references/guidelines.md b/agents/skills/dc-import-code-review/references/guidelines.md new file mode 100644 index 0000000000..9aea7f9020 --- /dev/null +++ b/agents/skills/dc-import-code-review/references/guidelines.md @@ -0,0 +1,85 @@ +# Import code review guidelines + +Apply only recommendations relevant to the changed import behavior. Current +repository contracts and instructions take precedence. + +## Manifest and automation + +- Validate the selected manifest specification against the current shared + manifest contract, and verify that every referenced script and input exists. +- Use `source_files` for source artifacts that must be retained; do not confuse + source artifacts with import inputs. +- Add `cron_schedule`, `user_script_timeout`, and `resource_limits` only when + the import needs scheduling or an override, and validate them when present. +- Keep curator contacts valid without prescribing one fixed email value. + +## Documentation and organization + +- Document the source, dataset coverage, prerequisites, working directory, + download and processing steps, important files, testing, and refresh + procedure in `README.md`. +- Preserve downloaded source files unchanged, and write transformed data to + separate files. +- Use consistent, descriptive names for new import files; prefer lowercase + unless source naming or an established import convention requires otherwise. +- Keep test inputs and expected CSV or TMCF outputs clearly paired without + requiring one universal test-directory layout. +- Declare new dependencies in the repository-supported dependency file. + +## Execution and failure handling + +- Keep module imports side-effect free by putting executable script logic behind + a guarded `main` entry point. +- Resolve import file paths relative to the script location rather than the + repository working directory. +- Ensure critical download or processing failures propagate and produce a + failing job rather than partial success. +- Catch specific exceptions only when handling or enriching them; preserve the + original traceback and include safe operational context. +- Check HTTP responses and external-command exit status before accepting their + output. +- Make directory creation and repeated execution safe, and do not expose + incomplete output as successful output. +- Use structured logging for operational progress and failures; do not assume + that a logging severity terminates execution. + +## Data transformation + +- Verify that StatisticalVariable names, mappings, units, and generated schema + output remain consistent with the transformation. +- Make aggregation, filtering, outlier handling, and date-range decisions + explicit and testable; avoid arbitrary future-year cutoffs. + +## Import validation + +- Configure `stat_var_processor` invocations to persist output counters for + validation. + +When reviewing `validation_config*.json` or a manifest change to +`validation_config_file`, read: + +- [Import validation framework](../../../../tools/import_validation/README.md) +- [Validation configuration and golden checks](../../../../tools/import_validation/Validations.md) + +## Download and processing reliability + +- Use `download_file` from the shared + [download utility](../../../../util/download_util_script.py) for HTTP(S) file + downloads instead of implementing download logic in individual imports. If + required behavior is missing, extend the shared utility when the capability + is reusable; use import-specific logic only for genuinely source-specific + behavior. +- Consume every page from paginated sources. +- Bound requests and retries with timeouts, limited attempts, and backoff; + distinguish transient failures from permanent ones. +- Make resume behavior idempotent, and ensure counters count unique successful + work across retries. +- Publish outputs atomically so partial downloads or transformations cannot + appear successful. + +## Tests + +- Test important success, failure, retry, pagination, and data-transformation + paths with representative fixtures. +- Keep checked-in fixtures representative and generally no more than 100 + records. diff --git a/agents/skills/dc-import-review-signal-miner/SKILL.md b/agents/skills/dc-import-review-signal-miner/SKILL.md new file mode 100644 index 0000000000..f67eeffb85 --- /dev/null +++ b/agents/skills/dc-import-review-signal-miner/SKILL.md @@ -0,0 +1,450 @@ +--- +name: dc-import-review-signal-miner +description: >- + Collects and classifies review comments from merged datacommonsorg/data pull + requests that touch scripts/** or statvar_imports/**. Use when bootstrapping + or incrementally mining strong positive and corrective import-review signals, + optionally limited to specified GitHub reviewers. Do not use to review a + single change or to update import-review guidelines. +--- + +# Mine Data Commons import review signals + +Collect review comments from merged pull requests in `datacommonsorg/data`, +identify strong import-review signals, and write the two Markdown reports +defined below. Do not update guidelines, upload artifacts, or create a pull +request. + +## Inputs + +- Start time, inclusive: `` in ISO 8601 UTC. +- End time, exclusive: `` in ISO 8601 UTC. +- Output directory: ``. +- Reviewer identities, optional: `` as comma-separated GitHub + logins, numeric user IDs, or both. + +If a required input is unresolved, ask for it before collecting data. If +`REVIEWERS` is omitted or empty, consider comments from every human reviewer. +Strip a leading `@` from logins and match logins case-insensitively. Match +numeric IDs exactly. Use `datacommonsorg/data` as the repository and `master` +as its current-code reference. + +## Check prerequisites + +From the Data Commons data repository root, run the skill-owned checker once +before creating temporary files or calling GitHub: + +```bash +bash agents/skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh +``` + +Stop if it exits nonzero. The checker verifies `git`, `gh`, GitHub CLI +authentication, and the exact `gh api` and `gh search prs` options used below. +It reports the detected `gh` version for provenance. It does not require the +standalone `jq` program because `gh --jq` provides the required filtering. + +## Scope and safety + +- Use `gh` for every GitHub operation. +- Process only pull requests whose `merged_at` value is within the requested + half-open interval and whose final changed-file list contains at least one + path under `scripts/**` or `statvar_imports/**`. +- Collect all non-empty inline review comments, submitted review bodies, and + pull request conversation comments from each eligible pull request. +- If `REVIEWERS` is provided, allow only comments authored by a matching + reviewer to have disposition `Considered`. Keep other comments in the + complete report as context and mark them `Not considered`. Author replies + and comments from other reviewers may still provide outcome evidence. +- Use comments on out-of-scope files only as surrounding evidence. Never + promote them as import-guideline signals. +- Treat the Data Commons checkout as read-only. +- Do not modify `guidelines.md`, upload files, post comments, or create or + update pull requests. +- Prefer skipping a possible signal over promoting an ambiguous signal. +- Do not expose credentials or authentication output in either report. + +## Prepare current master + +Use a current `master` snapshot only to verify that a candidate still matches +the repository. Do not create a checkout for each pull request. + +If a suitable local clone is available, fetch `master` and create one detached +temporary worktree at the fetched commit. Do not change the active worktree. + +Otherwise, create one temporary clone with `gh repo clone`. Make it shallow, +partial, and sparse: + +```bash +gh repo clone datacommonsorg/data /data -- \ + --depth=1 --filter=blob:none --sparse +git -C /data sparse-checkout set scripts statvar_imports +``` + +Create the temporary root with `mktemp -d` and record its resolved path before +using it. When using a worktree, remove it with `git worktree remove`. Never +remove an unverified path or the active repository. + +Verify that the snapshot resolves to the fetched `master` commit. A worktree +provides isolation; the shallow, partial, and sparse options reduce downloaded +data. Remove only temporary paths created by this run after both reports have +been written successfully. + +## Collect pull requests and comments + +Use GitHub REST API version `2026-03-10`. Do not silently fall back to an +unversioned request. If GitHub returns `410 Gone` for this version, stop and +report that the skill's API contract needs updating. + +After the prerequisite checker passes, use `--method GET` whenever passing +`-f` or `-F` query parameters; otherwise `gh api` changes the request to +`POST`. Use `--paginate --jq`, not `--paginate --slurp --jq`; the tested GitHub +CLI rejects the latter combination. + +Set task-specific shell variables before calling GitHub: + +```bash +DC_REPO='datacommonsorg/data' +GITHUB_API_VERSION='2026-03-10' +DC_SEARCH_START_DATE='' +DC_SEARCH_END_DATE='' +DC_RUN_DIR='/github' +DC_CANDIDATES_FILE="${DC_RUN_DIR}/candidate-prs.jsonl" +mkdir -p "${DC_RUN_DIR}" +: > "${DC_CANDIDATES_FILE}" +``` + +### Discover merged pull requests + +Count the coarse date-range search before retrieving candidates: + +```bash +DC_SEARCH_QUERY="repo:${DC_REPO} is:pr is:merged merged:${DC_SEARCH_START_DATE}..${DC_SEARCH_END_DATE}" +gh api --method GET \ + -H 'Accept: application/vnd.github+json' \ + -H "X-GitHub-Api-Version: ${GITHUB_API_VERSION}" \ + /search/issues \ + -f q="${DC_SEARCH_QUERY}" \ + -F per_page=1 \ + --jq '.total_count' +``` + +If the count exceeds 1,000, split the UTC date range into smaller ranges and +repeat. GitHub Search exposes at most 1,000 results for one query. Its date +range is inclusive, so deduplicate pull request numbers when coarse ranges +share a boundary date. + +For each range whose count is at most 1,000, collect candidates: + +```bash +gh search prs \ + --repo "${DC_REPO}" \ + --merged \ + --merged-at "${DC_SEARCH_START_DATE}..${DC_SEARCH_END_DATE}" \ + --limit 1000 \ + --json number,title,url \ + --jq '.[]' >> "${DC_CANDIDATES_FILE}" +``` + +After deduplication, retrieve authoritative metadata for every candidate: + +```bash +DC_PR_NUMBER='' +DC_PR_DIR="${DC_RUN_DIR}/pr-${DC_PR_NUMBER}" +mkdir -p "${DC_PR_DIR}" + +gh pr view "${DC_PR_NUMBER}" \ + --repo "${DC_REPO}" \ + --json number,title,url,mergedAt,mergeCommit,headRefOid,baseRefOid,author,changedFiles \ + --jq '{number,title,url,mergedAt, + merge_commit_sha:(.mergeCommit.oid // null), + head_sha:.headRefOid, + base_sha:.baseRefOid, + author_login:.author.login, + changed_files:.changedFiles}' \ + > "${DC_PR_DIR}/metadata.json" +``` + +Retain only metadata satisfying `START_TIME <= mergedAt < END_TIME`. The search +date range is only a coarse candidate filter; never use it as the final time +test. + +### Fetch changed files + +Fetch the complete changed-file list before collecting comments: + +```bash +gh api --method GET \ + -H 'Accept: application/vnd.github+json' \ + -H "X-GitHub-Api-Version: ${GITHUB_API_VERSION}" \ + "/repos/${DC_REPO}/pulls/${DC_PR_NUMBER}/files" \ + -F per_page=100 \ + --paginate \ + --jq '.[]' > "${DC_PR_DIR}/files.jsonl" +``` + +Retain the pull request only when `filename` or `previous_filename` begins with +`scripts/` or `statvar_imports/`. Compare the number of file records with +`changed_files` from `metadata.json`. The REST endpoint returns at most 3,000 +files. If the counts differ or `changed_files` exceeds 3,000, do not consider +signals from that pull request and record the collection limitation. + +### Fetch the three comment sources + +Fetch all inline review comments and their replies: + +```bash +gh api --method GET \ + -H 'Accept: application/vnd.github+json' \ + -H "X-GitHub-Api-Version: ${GITHUB_API_VERSION}" \ + "/repos/${DC_REPO}/pulls/${DC_PR_NUMBER}/comments" \ + -F per_page=100 \ + --paginate \ + --jq '.[]' > "${DC_PR_DIR}/review-comments.jsonl" +``` + +Fetch submitted review bodies. Exclude empty bodies, but retain every review +state, including `APPROVED`, `CHANGES_REQUESTED`, `COMMENTED`, and `DISMISSED`: + +```bash +gh api --method GET \ + -H 'Accept: application/vnd.github+json' \ + -H "X-GitHub-Api-Version: ${GITHUB_API_VERSION}" \ + "/repos/${DC_REPO}/pulls/${DC_PR_NUMBER}/reviews" \ + -F per_page=100 \ + --paginate \ + --jq '.[] | select((.body // "") != "")' \ + > "${DC_PR_DIR}/reviews.jsonl" +``` + +Fetch non-empty pull request conversation comments. GitHub exposes these +through the issue-comments endpoint because every pull request is also an +issue: + +```bash +gh api --method GET \ + -H 'Accept: application/vnd.github+json' \ + -H "X-GitHub-Api-Version: ${GITHUB_API_VERSION}" \ + "/repos/${DC_REPO}/issues/${DC_PR_NUMBER}/comments" \ + -F per_page=100 \ + --paginate \ + --jq '.[] | select((.body // "") != "")' \ + > "${DC_PR_DIR}/conversation-comments.jsonl" +``` + +The three files above are the complete comment sources for this workflow. +Standalone commit comments are out of scope. Fetch pull request commits and +the final diff only when needed to verify a possible signal: + +```bash +gh api --method GET \ + -H 'Accept: application/vnd.github+json' \ + -H "X-GitHub-Api-Version: ${GITHUB_API_VERSION}" \ + "/repos/${DC_REPO}/pulls/${DC_PR_NUMBER}/commits" \ + -F per_page=100 \ + --paginate \ + --jq '.[]' > "${DC_PR_DIR}/commits.jsonl" + +gh pr diff "${DC_PR_NUMBER}" \ + --repo "${DC_REPO}" \ + > "${DC_PR_DIR}/final.diff" +``` + +The pull-request commits endpoint returns at most 250 commits. Do not use an +apparently complete `commits.jsonl` as proof when the outcome depends on older +commits that may be omitted. + +If exact outcome evidence remains unavailable, mark the possible signal `Not +considered`. Do not broaden to undocumented endpoints or browser scraping. + +### Normalize collected records + +Use these fields from the downloaded JSON records: + +- Match reviewer filters against `user.login` and `user.id`. Use `user.type` + to exclude bots from considered signals. +- Link inline threads with `pull_request_review_id` and `in_reply_to_id`. +- Preserve `path`, `line`, `original_line`, `side`, `original_side`, + `diff_hunk`, `commit_id`, and `original_commit_id` when present. +- Preserve `body`, `created_at`, `updated_at`, `html_url`, and + `author_association` for every comment. +- Use the PR author's login from `metadata.json` to distinguish author replies + from reviewer-authored signal sources. + +Deduplicate by comment source and numeric `id`. Do not collapse distinct +comments with identical text. If any paginated command exits nonzero, stop and +report the incomplete endpoint instead of producing a successful run. + +The fixed endpoint contract is documented by GitHub's +[API versions](https://docs.github.com/en/rest/about-the-rest-api/api-versions), +[search](https://docs.github.com/en/rest/search/search), +[pull request](https://docs.github.com/en/rest/pulls/pulls), +[review comment](https://docs.github.com/en/rest/pulls/comments), +[review](https://docs.github.com/en/rest/pulls/reviews), and +[issue comment](https://docs.github.com/en/rest/issues/comments) references. + +## Classify every collected comment + +Classify each comment as one of: + +- `Positive signal`: it explicitly endorses a concrete import practice. +- `Corrective signal`: it explicitly requests or explains a concrete import + correction. +- `No signal`: it does not express a reusable import practice. + +Then assign one disposition: + +- `Considered`: strong enough to appear in the considered-signals projection. +- `Not considered`: insufficient for guideline consideration. + +Mark a signal `Considered` only when all of the following are established: + +- The signal concerns code under `scripts/**` or `statvar_imports/**`. +- The comment author matches `REVIEWERS` when that filter is provided. +- The desired behavior is clear and actionable. +- The practice applies to more than the source, dataset, or temporary + situation in that pull request. +- The pull request outcome supports the signal. For a corrective signal, the + requested behavior was implemented and merged. For a positive signal, the + endorsed behavior was retained in the merged result. +- The current `master` snapshot contains supporting code or documentation and + does not contradict the practice. +- The signal can be restated as a concise recommendation without guessing at + the reviewer's intent. + +Mark a comment `Not considered` when it is a question, generic approval, +automated message, personal style preference, one-off detail, unresolved +discussion, unimplemented suggestion, out-of-scope observation, obsolete +practice, or otherwise ambiguous. State the concrete reason. Do not invent +missing rationale. + +Treat reviewer comments as evidence rather than authority. A merged pull +request alone does not prove that every comment in it is valid. Use the comment +thread, resulting change, merged state, and current `master` together. +If current `master` cannot be verified, mark possible signals `Not considered` +and state that current validation was unavailable. + +## Phrase considered recommendations + +For every considered signal, write one short recommendation describing the +desired behavior. Phrase positive and corrective signals in the same neutral +form. + +Do not assign guideline IDs, severity, confidence scores, or mandatory rule +metadata. Do not combine unrelated signals. Preserve separate source comments +when several comments support the same recommendation. + +## Write the complete comments report + +Write +`/import-review-comments--.md`. +Include every collected comment from every eligible pull request, including +comments marked `Not considered`. +Create the output directory if needed, but do not create other persistent +files. + +Use this structure: + +```markdown +# Import review comments + +## Collection summary + +- Repository: datacommonsorg/data +- Interval: to +- Reviewer filter: All human reviewers | +- Current-code reference: +- Merged pull requests discovered: +- Eligible pull requests: +- Comments collected: +- Considered positive signals: +- Considered corrective signals: +- Comments not considered: +- Collection limitations: None | + +## PR - + +- Pull request: <URL> +- Merged at: <TIMESTAMP> +- Merge commit: <SHA> | Unavailable +- Pull request head commit: <SHA> +- In-scope changed paths: <PATHS> + +### Comment <COMMENT_URL> + +- Type: Inline review comment | Review body | Conversation comment +- Author: <LOGIN> (<NUMERIC USER ID>) +- Created at: <TIMESTAMP> +- Location: <PATH:LINE> | Pull request level +- Scope: Import path | Outside import path | Pull request level +- Signal: Positive signal | Corrective signal | No signal +- Disposition: Considered | Not considered +- Reason: <ONE CONCRETE SENTENCE> +- Proposed recommendation: <TEXT> | Not applicable +- Outcome evidence: <LINKS OR CURRENT MASTER PATHS> | None + +#### Comment text + +<VERBATIM COMMENT BODY> +``` + +Order pull requests by merge time and number. Within each pull request, order +comments by creation time, comment type, and numeric ID so repeated runs are +stable. + +## Write the considered-signals projection + +Write +`<OUTPUT_DIRECTORY>/import-review-signals-<START_DATE>-<END_DATE>.md`. +Derive this file from the completed comments report rather than classifying the +comments again. Include only entries whose disposition is `Considered`. + +Group entries with the same proposed recommendation when the evidence supports +the same general practice. Preserve every supporting comment URL and whether +each source was positive or corrective. + +Use this structure: + +```markdown +# Considered import review signals + +## Summary + +- Repository: datacommonsorg/data +- Interval: <START_TIME> to <END_TIME> +- Reviewer filter: All human reviewers | <NORMALIZED REVIEWER IDENTITIES> +- Current-code reference: <MASTER_SHA> +- Recommendations: <COUNT> +- Positive source comments: <COUNT> +- Corrective source comments: <COUNT> + +## <CONCISE RECOMMENDATION> + +- Recommendation: <DESIRED BEHAVIOR> +- Signal types: Positive | Corrective | Positive and corrective +- Why it is generalizable: <ONE OR TWO SENTENCES> +- Pull request outcome: <MERGED BEHAVIOR AND SUPPORTING LINKS> +- Current master evidence: <PATHS AND RELEVANT LINES OR SYMBOLS> +- Source comments: + - <POSITIVE OR CORRECTIVE> - <COMMENT URL> - <SHORT CONTEXT> +``` + +If there are no considered signals, still write the projection with a zero +count and the statement `No strong import-review signals were found.` + +## Validate and report completion + +Before finishing: + +- Confirm every retained pull request touches an import path. +- Confirm all paginated comment sources were exhausted. +- Confirm every considered source comment matches the reviewer filter when one + was provided. +- Confirm every comment in the projection exists in the complete report and is + marked `Considered`. +- Confirm the summary counts match the report contents. +- Confirm ambiguous signals were not promoted. +- Confirm only the two requested Markdown reports were created outside the + temporary checkout. + +Return the two output paths and the collection summary. Clearly report any +collection limitation; do not describe an incomplete run as successful. diff --git a/agents/skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh b/agents/skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh new file mode 100644 index 0000000000..25f7df6261 --- /dev/null +++ b/agents/skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh @@ -0,0 +1,56 @@ +#!/bin/bash + +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -uo pipefail + +TESTED_GH_VERSION='2.74.2' + +function fail { + echo "FAILED $1" >&2 + exit 1 +} + +command -v git >/dev/null 2>&1 || fail 'git is required' +command -v gh >/dev/null 2>&1 || fail 'GitHub CLI (gh) is required' + +gh_version="$(gh --version 2>/dev/null)" || fail 'gh --version' +gh_version="${gh_version%%$'\n'*}" +[[ -n "$gh_version" ]] || fail 'gh --version returned no output' + +api_help="$(gh api --help 2>&1)" || fail 'gh api --help' +for capability in '--paginate' '--jq'; do + if [[ "$api_help" != *"$capability"* ]]; then + fail "gh api does not support $capability" + fi +done + +search_help="$(gh search prs --help 2>&1)" || fail 'gh search prs --help' +for capability in '--merged' '--merged-at'; do + if [[ "$search_help" != *"$capability"* ]]; then + fail "gh search prs does not support $capability" + fi +done + +if ! gh auth status --hostname github.com >/dev/null 2>&1; then + fail 'gh is not authenticated for github.com' +fi + +echo 'PASS git and GitHub CLI are available' +echo "PASS $gh_version" +echo 'PASS Required gh capabilities' +echo 'PASS GitHub CLI authentication' +echo "INFO Workflow tested with GitHub CLI $TESTED_GH_VERSION" +echo 'INFO Standalone jq is not required; gh provides --jq' diff --git a/tools/import_validation/Validations.md b/tools/import_validation/Validations.md index 8a73c6e247..9ac32fd47f 100644 --- a/tools/import_validation/Validations.md +++ b/tools/import_validation/Validations.md @@ -3,9 +3,9 @@ The default validations in [validation_config.json](validation_config.json) are applied for all imports in auto refresh. -To add additional import specific validations, create a validation_config.json -in the import script folder and add it to the -config_overrides.validation_config_file parameter in the manifest.json. +To add import-specific validations, create a `validation_config.json` in the +import directory and set `validation_config_file` on the relevant import +specification in `manifest.json` to its import-relative path. To override or disable a default validation rule, copy the rule to the import specific config with the same rule id and @@ -27,7 +27,7 @@ disable lint check for a specific import. }, { "rule_id": "check_lint_error_count", - "enabled": false, + "enabled": false } ] } @@ -46,9 +46,12 @@ in the input, the validation is treated as a failure. The missing golden rows are listed in the validation report json. ### Configuration Parameters -- `golden_files`: A list or glob pattern of golden MCF or CSV files to compare against. +- `golden_files`: A path, glob pattern, or list of paths or patterns for golden + MCF or CSV files to compare against. - `goldens_key_property`: A list of properties to match on. If not specified, all properties in the golden record must match. -- `input_files`: (Optional) A list of glob pattern of input files to be compared with goldens. If not provided, the data source defined in the rule's `scope` is used. +- `input_files`: (Optional) A path, glob pattern, or list of paths or patterns + for input files to compare with goldens. If not provided, the data source + defined in the rule's `scope` is used. ### GOLDENS_CHECK Validator Example @@ -103,7 +106,7 @@ place dcids loaded from txt files: To enable goldens validation with files generated above while relaxing the default deleted records threshold, add the following -valiation rules to the validation config: +validation rules to the validation config: ```json { @@ -128,7 +131,7 @@ valiation rules to the validation config: "rule_id": "check_golden_observations_statvar_places_dates", "validator": "GOLDENS_CHECK", "params": { - "golden_files": "golden_data/golden_observations.csv" + "golden_files": "golden_data/golden_observations.csv", "input_files": "output/observations.csv" } } @@ -137,5 +140,3 @@ valiation rules to the validation config: ``` - -