Added import code review agent skill

.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"      
     }

agents/README.md

@@ -6,10 +6,53 @@ 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
 [`dc-import-diagnostics` starter prompt](prompts/dc-import-diagnostics-starter.md).
 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.

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('<REPO_ROOT>/', text)
+
+
 if __name__ == '__main__':
     unittest.main()

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.

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-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
+```

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
+
+<IMPORT_REVIEW_REQUEST>

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: `<START_TIME>` in ISO 8601 UTC.
+- End time, exclusive: `<END_TIME>` in ISO 8601 UTC.
+- Output directory: `<OUTPUT_DIRECTORY>`.
+- Reviewer identities, optional: `<REVIEWERS>` 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.