From 049efc288e49d7e7fc0c08acce5ed830867e01de Mon Sep 17 00:00:00 2001 From: Rohit Kumar Date: Fri, 14 Aug 2026 11:52:31 +0530 Subject: [PATCH 01/12] Add import review signal mining skill --- agents/README.md | 23 + .../dc-import-review-signal-miner-starter.md | 13 + .../dc-import-review-signal-miner/SKILL.md | 450 ++++++++++++++++++ .../scripts/check_prerequisites.sh | 56 +++ 4 files changed, 542 insertions(+) create mode 100644 agents/prompts/dc-import-review-signal-miner-starter.md create mode 100644 agents/skills/dc-import-review-signal-miner/SKILL.md create mode 100644 agents/skills/dc-import-review-signal-miner/scripts/check_prerequisites.sh diff --git a/agents/README.md b/agents/README.md index 6e554d21a8..3643030bbf 100644 --- a/agents/README.md +++ b/agents/README.md @@ -13,3 +13,26 @@ 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. + +## 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. 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/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' From 82d2679102c072ba5d8870371e0e2bd82abf1de3 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Fri, 14 Aug 2026 13:48:22 +0530 Subject: [PATCH 02/12] Add Data Commons import review skill --- .agents/skills.json | 3 + agents/README.md | 8 + agents/common/scripts/skill_contract_test.py | 73 +++++++ .../prompts/dc-import-code-review-starter.md | 12 ++ agents/skills/dc-import-code-review/SKILL.md | 198 ++++++++++++++++++ .../references/guidelines.md | 60 ++++++ 6 files changed, 354 insertions(+) create mode 100644 agents/prompts/dc-import-code-review-starter.md create mode 100644 agents/skills/dc-import-code-review/SKILL.md create mode 100644 agents/skills/dc-import-code-review/references/guidelines.md 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 3643030bbf..3953dd407e 100644 --- a/agents/README.md +++ b/agents/README.md @@ -14,6 +14,14 @@ 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 diff --git a/agents/common/scripts/skill_contract_test.py b/agents/common/scripts/skill_contract_test.py index cf2166dbfb..77b9ff68dc 100644 --- a/agents/common/scripts/skill_contract_test.py +++ b/agents/common/scripts/skill_contract_test.py @@ -428,5 +428,78 @@ 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_scope_safety_and_output_contract(self): + skill = self._skill_path.read_text(encoding='utf-8') + normalized = re.sub(r'\s+', ' ', skill) + + for contract in ( + 'Report findings only for changed files under `scripts/**` and `statvar_imports/**`', + 'Treat the repository and GitHub as read-only', + 'If the review target is ambiguous', + 'Never run `gh pr checkout` over the active worktree', + 'Do not run tests that call live source, Data Commons, or cloud APIs', + 'references/guidelines.md', + '../../common/references/import-automation/manifest.md', + ): + with self.subTest(contract=contract): + self.assertIn(contract, normalized) + + for heading in ('## Review scope', '## Findings', + '## Positive findings', '## Coverage', + '## Verification and limitations'): + with self.subTest(heading=heading): + self.assertIn(heading, skill) + + for priority in ('| P0 |', '| P1 |', '| P2 |', '| P3 |'): + with self.subTest(priority=priority): + self.assertIn(priority, skill) + + for field in ('Finding', 'Impact', 'Recommendation'): + with self.subTest(field=field): + self.assertIn(f'- {field}:', skill) + + self.assertIn('Finding: Good - <WHAT WAS DONE CORRECTLY>', skill) + self.assertIn('| File | Status | Result |', skill) + + 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) + + if __name__ == '__main__': unittest.main() 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..312014bcd8 --- /dev/null +++ b/agents/prompts/dc-import-code-review-starter.md @@ -0,0 +1,12 @@ +# 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 and GitHub as read-only. + +## Request + +<IMPORT_REVIEW_REQUEST> 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..dae455fa29 --- /dev/null +++ b/agents/skills/dc-import-code-review/SKILL.md @@ -0,0 +1,198 @@ +--- +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 and GitHub +unchanged. + +## Safety and scope + +- Resolve the repository root with `git rev-parse --show-toplevel`. Verify that + `<REPO_ROOT>/scripts/` and `<REPO_ROOT>/statvar_imports/` exist, and run + repository-relative Git operations from `<REPO_ROOT>`. +- Treat the repository and GitHub as read-only. Never edit the import, post or + resolve review comments, approve a pull request, stage files, discard local + changes, or change the active branch. +- 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 <BASE_REF> 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 <PR> --repo datacommonsorg/data \ + --json number,title,url,baseRefName,baseRefOid,headRefOid,changedFiles,additions,deletions,files +gh pr diff <PR> --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. +- 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. 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). +The shared reference is authoritative 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. + +## 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: <staged | unstaged | all local | branch comparison | PR> +- Reviewed: <in-scope files and changed hunks> +- Skipped: <out-of-scope changed files, if any> + +## Findings + +### [P1] <Category> + +- 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: <commands or none> +- Checks not run: <reason> +- Limitations: <missing context or none> +``` + +Order actionable findings by P0 through P3. Every actionable finding must +include `Finding`, `Impact`, and `Recommendation`. Every positive finding must +include `Finding: Good - <WHAT WAS DONE CORRECTLY>`. Use the exact `File`, +`Status`, and `Result` coverage columns shown above, and include every in-scope +changed file, including files with no findings. 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..6aaeeae3de --- /dev/null +++ b/agents/skills/dc-import-code-review/references/guidelines.md @@ -0,0 +1,60 @@ +# 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`. +- 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 + +- 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. + +## Download and processing reliability + +- 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. From 01237856811f004ac08d48124e64ea9c86ec1aba Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Fri, 14 Aug 2026 14:11:52 +0530 Subject: [PATCH 03/12] Clarify import validation guidance and review contracts --- agents/common/scripts/skill_contract_test.py | 56 +++++++++---------- agents/skills/dc-import-code-review/SKILL.md | 19 ++++--- .../references/guidelines.md | 8 +++ tools/import_validation/Validations.md | 21 +++---- 4 files changed, 58 insertions(+), 46 deletions(-) diff --git a/agents/common/scripts/skill_contract_test.py b/agents/common/scripts/skill_contract_test.py index 77b9ff68dc..0eff27df4a 100644 --- a/agents/common/scripts/skill_contract_test.py +++ b/agents/common/scripts/skill_contract_test.py @@ -451,38 +451,19 @@ def test_review_skill_is_registered_and_discoverable(self): self.assertIn('prompts/dc-import-code-review-starter.md', readme) self.assertIn('`dc-import-code-review` skill', prompt) - def test_review_skill_keeps_scope_safety_and_output_contract(self): + def test_review_skill_keeps_core_contract(self): skill = self._skill_path.read_text(encoding='utf-8') - normalized = re.sub(r'\s+', ' ', skill) - for contract in ( - 'Report findings only for changed files under `scripts/**` and `statvar_imports/**`', - 'Treat the repository and GitHub as read-only', - 'If the review target is ambiguous', - 'Never run `gh pr checkout` over the active worktree', - 'Do not run tests that call live source, Data Commons, or cloud APIs', - 'references/guidelines.md', - '../../common/references/import-automation/manifest.md', - ): - with self.subTest(contract=contract): - self.assertIn(contract, normalized) - - for heading in ('## Review scope', '## Findings', - '## Positive findings', '## Coverage', - '## Verification and limitations'): - with self.subTest(heading=heading): - self.assertIn(heading, skill) + for marker in ('scripts/**', 'statvar_imports/**', 'read-only', 'P0', + 'P1', 'P2', 'P3', 'Finding', 'Impact', 'Recommendation'): + with self.subTest(marker=marker): + self.assertIn(marker, skill) - for priority in ('| P0 |', '| P1 |', '| P2 |', '| P3 |'): - with self.subTest(priority=priority): - self.assertIn(priority, skill) - - for field in ('Finding', 'Impact', 'Recommendation'): - with self.subTest(field=field): - self.assertIn(f'- {field}:', skill) - - self.assertIn('Finding: Good - <WHAT WAS DONE CORRECTLY>', skill) - self.assertIn('| File | Status | Result |', 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 / @@ -500,6 +481,23 @@ def test_review_guidance_stays_single_and_lightweight(self): 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('<REPO_ROOT>/', text) + if __name__ == '__main__': unittest.main() diff --git a/agents/skills/dc-import-code-review/SKILL.md b/agents/skills/dc-import-code-review/SKILL.md index dae455fa29..2542b4c09c 100644 --- a/agents/skills/dc-import-code-review/SKILL.md +++ b/agents/skills/dc-import-code-review/SKILL.md @@ -11,9 +11,9 @@ unchanged. ## Safety and scope -- Resolve the repository root with `git rev-parse --show-toplevel`. Verify that - `<REPO_ROOT>/scripts/` and `<REPO_ROOT>/statvar_imports/` exist, and run - repository-relative Git operations from `<REPO_ROOT>`. +- 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 and GitHub as read-only. Never edit the import, post or resolve review comments, approve a pull request, stage files, discard local changes, or change the active branch. @@ -104,17 +104,22 @@ limitation instead of claiming complete coverage. ## Load review guidance Read [import code review guidelines](references/guidelines.md) for every -review. Apply a recommendation only when it is relevant to the changed -behavior. +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). -The shared reference is authoritative for current fields and requirements; do -not reconstruct the manifest contract from historical guidance. +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 diff --git a/agents/skills/dc-import-code-review/references/guidelines.md b/agents/skills/dc-import-code-review/references/guidelines.md index 6aaeeae3de..c998038466 100644 --- a/agents/skills/dc-import-code-review/references/guidelines.md +++ b/agents/skills/dc-import-code-review/references/guidelines.md @@ -44,6 +44,14 @@ repository contracts and instructions take precedence. - Make aggregation, filtering, outlier handling, and date-range decisions explicit and testable; avoid arbitrary future-year cutoffs. +## Import 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 - Consume every page from paginated sources. 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: ``` - - From 9d11e9c71b1913dc172e74a0062acfd308ef041f Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Fri, 14 Aug 2026 15:00:28 +0530 Subject: [PATCH 04/12] Document import review signal merge workflow --- agents/README.md | 8 ++ agents/prompts/import-review-signal-merge.md | 99 ++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 agents/prompts/import-review-signal-merge.md diff --git a/agents/README.md b/agents/README.md index 3953dd407e..fecfb54847 100644 --- a/agents/README.md +++ b/agents/README.md @@ -44,3 +44,11 @@ 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/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: `<SIGNAL_OUTPUT_DIRECTORY>` +- Data repository path: `<DATA_REPOSITORY_PATH>` + +Ask for either input if it is missing. Do not guess it. + +## Check the repository + +Resolve the repository root from `<DATA_REPOSITORY_PATH>`. 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 +`<SIGNAL_OUTPUT_DIRECTORY>`. 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 +`<SIGNAL_OUTPUT_DIRECTORY>/import-review-signal-merge-<YYYYMMDD-HHMMSS>.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 +### <SIGNAL RECOMMENDATION> + +- Disposition: Added | Already covered | Conflict | Skipped +- Source comments: <URLS FROM THE PROJECTION> +- Existing guidance: <MATCHING OR CONFLICTING GUIDELINE> | None +- Change: <ADDED GUIDELINE BULLET> | None +- Rationale: <SPECIFIC REASON FOR THE DECISION> +``` + +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. From db44eaff6326ce2b64bea2af320c250a836666a5 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Fri, 14 Aug 2026 15:18:56 +0530 Subject: [PATCH 05/12] Strengthen data import code review guidelines --- .../dc-import-code-review/references/guidelines.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/agents/skills/dc-import-code-review/references/guidelines.md b/agents/skills/dc-import-code-review/references/guidelines.md index c998038466..17f022d78a 100644 --- a/agents/skills/dc-import-code-review/references/guidelines.md +++ b/agents/skills/dc-import-code-review/references/guidelines.md @@ -18,6 +18,8 @@ repository contracts and instructions take precedence. - 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 @@ -26,6 +28,10 @@ repository contracts and instructions take precedence. ## 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 @@ -46,6 +52,9 @@ repository contracts and instructions take precedence. ## 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: @@ -66,3 +75,5 @@ When reviewing `validation_config*.json` or a manifest change to - 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. From d8bcc2eeefa587b1f396aae376b04388dd01a371 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Fri, 14 Aug 2026 15:30:53 +0530 Subject: [PATCH 06/12] Require isolated Python environments in import code reviews --- agents/skills/dc-import-code-review/SKILL.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/agents/skills/dc-import-code-review/SKILL.md b/agents/skills/dc-import-code-review/SKILL.md index 2542b4c09c..d64a3947e0 100644 --- a/agents/skills/dc-import-code-review/SKILL.md +++ b/agents/skills/dc-import-code-review/SKILL.md @@ -95,6 +95,13 @@ is needed: - 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 <repository-relative-test-directory>`; 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. From 1bfc2afaf2db63e82c892fea59076e21245fb3a9 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Sat, 15 Aug 2026 06:54:44 +0530 Subject: [PATCH 07/12] Document agent skill authoring conventions --- agents/README.md | 4 ++ agents/docs/dc-import-diagnostics.md | 63 +++++++++++++++++++ agents/docs/skill-authoring.md | 91 ++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 agents/docs/dc-import-diagnostics.md create mode 100644 agents/docs/skill-authoring.md diff --git a/agents/README.md b/agents/README.md index fecfb54847..7410675c0a 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.md). + ## Inspect or diagnose imports For read-only ET import information or diagnosis, use the diff --git a/agents/docs/dc-import-diagnostics.md b/agents/docs/dc-import-diagnostics.md new file mode 100644 index 0000000000..96f4948d83 --- /dev/null +++ b/agents/docs/dc-import-diagnostics.md @@ -0,0 +1,63 @@ +# 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. + +## Keep diagnosis hypothesis-driven + +```text +known symptom or minimal triage +→ failure domain +→ plausible hypothesis +→ verify, refute, or leave unknown +→ likely cause +→ mitigation or next action +``` + +Keep each hypothesis beside: + +- when to consider it; +- evidence that confirms or refutes it; and +- mitigation when confirmed. + +A user-supplied hypothesis changes investigation order but does not prove the +cause. Treat indirect signals as clues. Treat unavailable evidence as unknown, +not refuted. Test only plausible hypotheses and do not gather all evidence up +front. + +Domain guides own interpretation. Operational references, such as +[Cloud Batch operations](../skills/dc-import-diagnostics/references/batch.md), +own commands, identifiers, bounds, and evidence-retrieval failure handling. +When adding an evidence source, add its operation there first and link the +hypothesis to it. + +Do not repeat skill-wide safety or remediation policy in each troubleshooting +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..1af8709603 --- /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>/SKILL.md` | +| Skill-specific references | `agents/skills/<skill>/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.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 +``` From ad56c5535e1c7e3ca3f6d4c689a32b5cd27123a8 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Sat, 15 Aug 2026 07:12:55 +0530 Subject: [PATCH 08/12] Clarify import diagnostics authoring --- agents/README.md | 2 +- .../docs/dc-import-diagnostics-authoring.md | 95 +++++++++++++++++++ agents/docs/dc-import-diagnostics.md | 63 ------------ agents/docs/skill-authoring.md | 2 +- 4 files changed, 97 insertions(+), 65 deletions(-) create mode 100644 agents/docs/dc-import-diagnostics-authoring.md delete mode 100644 agents/docs/dc-import-diagnostics.md diff --git a/agents/README.md b/agents/README.md index 7410675c0a..d99ccecd09 100644 --- a/agents/README.md +++ b/agents/README.md @@ -8,7 +8,7 @@ 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.md). +[DC import diagnostics authoring](docs/dc-import-diagnostics-authoring.md). ## Inspect or diagnose imports 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/dc-import-diagnostics.md b/agents/docs/dc-import-diagnostics.md deleted file mode 100644 index 96f4948d83..0000000000 --- a/agents/docs/dc-import-diagnostics.md +++ /dev/null @@ -1,63 +0,0 @@ -# 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. - -## Keep diagnosis hypothesis-driven - -```text -known symptom or minimal triage -→ failure domain -→ plausible hypothesis -→ verify, refute, or leave unknown -→ likely cause -→ mitigation or next action -``` - -Keep each hypothesis beside: - -- when to consider it; -- evidence that confirms or refutes it; and -- mitigation when confirmed. - -A user-supplied hypothesis changes investigation order but does not prove the -cause. Treat indirect signals as clues. Treat unavailable evidence as unknown, -not refuted. Test only plausible hypotheses and do not gather all evidence up -front. - -Domain guides own interpretation. Operational references, such as -[Cloud Batch operations](../skills/dc-import-diagnostics/references/batch.md), -own commands, identifiers, bounds, and evidence-retrieval failure handling. -When adding an evidence source, add its operation there first and link the -hypothesis to it. - -Do not repeat skill-wide safety or remediation policy in each troubleshooting -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 index 1af8709603..260c2bce4a 100644 --- a/agents/docs/skill-authoring.md +++ b/agents/docs/skill-authoring.md @@ -68,7 +68,7 @@ when they differ. - Improve skills based on observed failures. For diagnostics-specific routing and troubleshooting conventions, see -[DC import diagnostics authoring](dc-import-diagnostics.md). +[DC import diagnostics authoring](dc-import-diagnostics-authoring.md). ## Validate changes From 576a5d4aab132044b4df9136af20b7ea0ab37782 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Sat, 15 Aug 2026 07:33:58 +0530 Subject: [PATCH 09/12] Discover skill link entrypoints from installed skills --- agents/common/scripts/skill_contract_test.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/agents/common/scripts/skill_contract_test.py b/agents/common/scripts/skill_contract_test.py index 0eff27df4a..c9a6f193b5 100644 --- a/agents/common/scripts/skill_contract_test.py +++ b/agents/common/scripts/skill_contract_test.py @@ -258,11 +258,8 @@ 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) From 0a0b0f6e48f9873309b5e14d8df6ca88230a714d Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Sat, 15 Aug 2026 11:14:12 +0530 Subject: [PATCH 10/12] lint fix --- agents/common/scripts/skill_contract_test.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/agents/common/scripts/skill_contract_test.py b/agents/common/scripts/skill_contract_test.py index c9a6f193b5..5bcc48c5e3 100644 --- a/agents/common/scripts/skill_contract_test.py +++ b/agents/common/scripts/skill_contract_test.py @@ -258,8 +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): - entrypoints = sorted( - (self._agents_root / 'skills').glob('*/SKILL.md')) + 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) From 6511158127be209064f77176a0e9c82f13208fc0 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Sat, 15 Aug 2026 18:30:54 +0530 Subject: [PATCH 11/12] Allow explicitly authorized review publishing --- .../prompts/dc-import-code-review-starter.md | 4 +- agents/skills/dc-import-code-review/SKILL.md | 75 +++++++++++++++++-- 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/agents/prompts/dc-import-code-review-starter.md b/agents/prompts/dc-import-code-review-starter.md index 312014bcd8..36fd5759da 100644 --- a/agents/prompts/dc-import-code-review-starter.md +++ b/agents/prompts/dc-import-code-review-starter.md @@ -5,7 +5,9 @@ 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 and GitHub as read-only. +- 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/skills/dc-import-code-review/SKILL.md b/agents/skills/dc-import-code-review/SKILL.md index d64a3947e0..c942fbaaea 100644 --- a/agents/skills/dc-import-code-review/SKILL.md +++ b/agents/skills/dc-import-code-review/SKILL.md @@ -6,17 +6,24 @@ description: Reviews staged, unstaged, branch-comparison, or GitHub pull request # 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 and GitHub -unchanged. +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 and GitHub as read-only. Never edit the import, post or - resolve review comments, approve a pull request, stage files, discard local - changes, or change the active branch. +- 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/**`. @@ -208,3 +215,61 @@ include `Finding`, `Impact`, and `Recommendation`. Every positive finding must include `Finding: Good - <WHAT WAS DONE CORRECTLY>`. 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/<PR_NUMBER>/reviews \ + --input <PAYLOAD_FILE> \ + --jq '{id, state, html_url, commit_id}' +``` + +Use this payload shape. Omit `comments` when there are no inline findings. + +```json +{ + "commit_id": "<VERIFIED_HEAD_SHA>", + "event": "COMMENT", + "body": "<REVIEW_SUMMARY>", + "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. From 7e56eea733028c504f11b8532c5e36d495d27cc4 Mon Sep 17 00:00:00 2001 From: Rohit Kumar <rohitkumarbhagat.in@gmail.com> Date: Sat, 15 Aug 2026 18:51:53 +0530 Subject: [PATCH 12/12] Document shared download utility guidance --- .../skills/dc-import-code-review/references/guidelines.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agents/skills/dc-import-code-review/references/guidelines.md b/agents/skills/dc-import-code-review/references/guidelines.md index 17f022d78a..9aea7f9020 100644 --- a/agents/skills/dc-import-code-review/references/guidelines.md +++ b/agents/skills/dc-import-code-review/references/guidelines.md @@ -63,6 +63,12 @@ When reviewing `validation_config*.json` or a manifest change to ## 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.