Add internal check

Author: luigi liu

awscli/customizations/agenttoolkit/state.py

@@ -147,14 +147,14 @@ def _is_valid_hook_run(run):
 
 
 def get_last_hook_run(path, state=None):
-    """Return the last run recorded for the hook at ``path``, or ``None``."""
+    """Return the last check recorded for ``path``, or ``None``."""
     return get_last_hook_runs(state).get(_hook_key(path))
 
 
 def record_hook_run(path, is_successful, summary, timestamp=None):
-    """Record the outcome of an automatic update run by the hook at ``path``.
+    """Record the outcome of an automatic update check of ``path``.
 
-    :param path: Harness config file the hook is installed in.
+    :param path: Skills root the hook run checked.
     :param timestamp: Aware ``datetime`` of the run. Defaults to now.
     """
     if timestamp is None:

awscli/customizations/agenttoolkit/update_skill.py

@@ -30,12 +30,12 @@
     HOOK_ERROR_MESSAGE,
     format_hook_message,
     format_hook_output,
-    hook_config_path,
 )
 from awscli.customizations.agenttoolkit.state import (
     StateFileError,
     get_last_hook_run,
     is_auto_update_enabled,
+    load_state,
     parse_timestamp,
     record_hook_run,
     toolkit_dir,
@@ -145,12 +145,41 @@ def summary(self):
 )
 
 # Several sessions can start at once, and an agent may run more than one
-# hook at session start. Hook runs take turns, and a run that starts right
-# after another for the same agent finished does nothing, so the agent hears
-# about an update once.
+# hook at session start. Hook runs take turns, so a run that starts right
+# after another finds its skills roots already checked and does nothing.
 HOOK_LOCK_FILENAME = 'hook.lock'
 HOOK_LOCK_TIMEOUT = 30
-HOOK_RECENT_RUN_SECONDS = 10
+
+# A hook run checks a skills root again once this long has passed since its
+# last check, or sooner if that check failed. Each root is tracked on its own,
+# so a check through one agent covers every agent sharing the root.
+HOOK_CHECK_INTERVAL = datetime.timedelta(hours=24)
+HOOK_RETRY_INTERVAL = datetime.timedelta(hours=1)
+
+
+def _is_check_due(last_run, now):
+    if last_run is None:
+        return True
+    checked = parse_timestamp(last_run['timestamp'])
+    if checked > now:
+        # The clock moved back, so the record can't be trusted.
+        return True
+    if last_run['is_successful']:
+        return now - checked >= HOOK_CHECK_INTERVAL
+    return now - checked >= HOOK_RETRY_INTERVAL
+
+
+def _root_succeeded(root, results):
+    """Whether no skill failed to update or install in skills ``root``."""
+    for name in results.failed_names:
+        # Still installed here, so its update was due here.
+        if os.path.exists(os.path.join(root, name, SKILL_METADATA_FILENAME)):
+            return False
+    for name in results.failed_install_names:
+        # Still missing here, so the install was meant for here.
+        if not os.path.lexists(os.path.join(root, name)):
+            return False
+    return True
 
 
 class UpdateSkillCommand(BasicCommand):
@@ -275,26 +304,40 @@ def _run_hook(self, agent_id, parsed_globals):
                 return 0
             except OSError as e:
                 LOG.debug('Could not lock the hook run: %s', e)
-            if self._hook_ran_recently(agent_id):
-                return 0
             return self._run_hook_locked(agent_id, parsed_globals)
 
-    def _hook_ran_recently(self, agent_id):
-        run = get_last_hook_run(hook_config_path(self._hook_config(agent_id)))
-        if run is None:
-            return False
-        age = datetime.datetime.now(datetime.timezone.utc) - parse_timestamp(
-            run['timestamp']
-        )
-        return age.total_seconds() < HOOK_RECENT_RUN_SECONDS
+    def _agents_due_for_check(self, agents):
+        """Return the agents with AWS skills whose skills root is due a check,
+        and those roots."""
+        now = datetime.datetime.now(datetime.timezone.utc)
+        state = load_state()
+        is_due_by_root = {}
+        due_agents = []
+        for agent in agents:
+            if not agent.get_installed_skills():
+                continue
+            root = os.path.realpath(agent.skills_path)
+            if root not in is_due_by_root:
+                is_due_by_root[root] = _is_check_due(
+                    get_last_hook_run(root, state), now
+                )
+            if is_due_by_root[root]:
+                due_agents.append(agent)
+        due_roots = [root for root, is_due in is_due_by_root.items() if is_due]
+        return due_agents, due_roots
 
     def _run_hook_locked(self, agent_id, parsed_globals):
         # The agent shows or acts on whatever is printed, and a failing hook
         # is reported as an error, so nothing escapes and it always succeeds.
         results = _UpdateResults.empty()
         error = None
+        due_roots = []
         try:
-            agents = resolve_agents(None, self._agent_configs)
+            agents, due_roots = self._agents_due_for_check(
+                resolve_agents(None, self._agent_configs)
+            )
+            if not agents:
+                return 0
             if self._collect_updates(
                 agents, parsed_globals, None, results, HOOK_CLIENT_CONFIG
             ):
@@ -314,11 +357,9 @@ def _run_hook_locked(self, agent_id, parsed_globals):
             results.failed_install_names,
         )
         if error is None:
-            is_successful = not (results.failed or results.failed_installs)
             summary = results.summary()
         else:
             message = f'{message} {HOOK_ERROR_MESSAGE}'.lstrip()
-            is_successful = False
             summary = f'Could not check for updates: {error}'
         self._stream.write(
             format_hook_output(
@@ -327,27 +368,22 @@ def _run_hook_locked(self, agent_id, parsed_globals):
                 skills_updated=bool(results.updated or results.installed),
             )
         )
-        self._record_hook_run(agent_id, is_successful, summary)
+        self._record_hook_runs(due_roots, results, error, summary)
         return 0
 
-    def _hook_config(self, agent_id):
-        # The hook's agent may not be detected, or not in a custom config
-        # list, but every hook id has a config in AGENT_CONFIGS.
-        return next(
-            c
-            for c in [*self._agent_configs, *AGENT_CONFIGS]
-            if c.id == agent_id
-        )
-
-    def _record_hook_run(self, agent_id, is_successful, summary):
-        try:
-            record_hook_run(
-                hook_config_path(self._hook_config(agent_id)),
-                is_successful=is_successful,
-                summary=summary,
-            )
-        except StateFileError as e:
-            LOG.debug('Could not record the hook run: %s', e)
+    def _record_hook_runs(self, roots, results, error, summary):
+        timestamp = datetime.datetime.now(datetime.timezone.utc)
+        for root in roots:
+            is_successful = error is None and _root_succeeded(root, results)
+            try:
+                record_hook_run(
+                    root,
+                    is_successful=is_successful,
+                    summary=summary,
+                    timestamp=timestamp,
+                )
+            except StateFileError as e:
+                LOG.debug('Could not record the hook run: %s', e)
 
     def _update_all_skills(self, agents, parsed_globals):
         results = _UpdateResults.empty()
@@ -404,9 +440,7 @@ def _collect_updates(
                 results.failed.append((skill_name, e))
                 continue
             if skill_skipped:
-                results.skipped.extend(
-                    (skill_name, *s) for s in skill_skipped
-                )
+                results.skipped.extend((skill_name, *s) for s in skill_skipped)
             else:
                 results.updated.append(skill_name)
         return True

tests/unit/customizations/agenttoolkit/test_update_skill.py

@@ -959,13 +959,9 @@ def _run_hook(tmp_path, agent_id, client, configs=None, extra_args=()):
     return rc, stream.getvalue()
 
 
-def _last_run(tmp_path, agent_id='claude-code'):
-    path = tmp_path / '.claude' / 'settings.json'
-    if agent_id != 'claude-code':
-        path = hooks.hook_config_path(
-            next(c for c in AGENT_CONFIGS if c.id == agent_id)
-        )
-    return state.get_last_hook_run(str(path))
+def _last_run(tmp_path, agent_dir='.test-agent'):
+    root = os.path.realpath(tmp_path / agent_dir / 'skills')
+    return state.get_last_hook_run(root)
 
 
 def test_hook_does_nothing_when_auto_update_off(tmp_path):
@@ -1028,8 +1024,8 @@ def test_hook_output_per_agent(tmp_path, auto_update_on, agent_id, expected):
         assert json.loads(output) == expected
     else:
         assert output == expected
-    # Recorded against the agent's hook file even though it isn't detected.
-    assert _last_run(tmp_path, agent_id)['is_successful'] is True
+    # Recorded against the skills root, whichever agent's hook ran.
+    assert _last_run(tmp_path)['is_successful'] is True
 
 
 def test_hook_prints_nothing_when_up_to_date(tmp_path, auto_update_on):
@@ -1044,9 +1040,11 @@ def test_hook_prints_nothing_when_up_to_date(tmp_path, auto_update_on):
 def test_hook_prints_nothing_without_skills_or_agents(
     tmp_path, auto_update_on
 ):
-    rc, output = _run_hook(tmp_path, 'claude-code', _make_skill_client())
+    client = _make_skill_client()
+    rc, output = _run_hook(tmp_path, 'claude-code', client)
     assert (rc, output) == (0, '')
-    assert _last_run(tmp_path)['is_successful'] is True
+    assert not client.get_latest_skill_version.called
+    assert state.get_last_hook_runs() == {}
 
 
 def test_hook_reports_failures(tmp_path, auto_update_on):
@@ -1059,7 +1057,7 @@ def test_hook_reports_failures(tmp_path, auto_update_on):
         'AWS agent skills updated: aws-cdk. Could not update aws-s3; run '
         '"aws agent-toolkit update-skill --all" for details.\n'
     )
-    run = _last_run(tmp_path, 'codex')
+    run = _last_run(tmp_path)
     assert run['is_successful'] is False
     assert run['summary'] == (
         '1 updated, 0 skipped, 1 failed, 0 already up to date'
@@ -1213,10 +1211,13 @@ def test_hook_record_failure_is_ignored(tmp_path, auto_update_on):
         rc, output = _run_hook(tmp_path, 'codex', _make_skill_client())
     assert rc == 0
     assert output == 'AWS agent skills updated: aws-s3.\n'
-    assert record.call_args.kwargs == {
-        'is_successful': True,
-        'summary': '1 updated, 0 skipped, 0 failed, 0 already up to date',
-    }
+    assert record.call_args.args == (
+        os.path.realpath(tmp_path / '.test-agent' / 'skills'),
+    )
+    assert record.call_args.kwargs['is_successful'] is True
+    assert record.call_args.kwargs['summary'] == (
+        '1 updated, 0 skipped, 0 failed, 0 already up to date'
+    )
 
 
 def test_hook_accepts_all(tmp_path, auto_update_on):
@@ -1336,7 +1337,7 @@ def test_hook_reports_core_skill_install_failure(tmp_path, auto_update_on):
         '--skill-name aws-iam" for details.\n'
     )
     assert not _skill_dir(tmp_path, 'aws-iam').exists()
-    run = _last_run(tmp_path, 'codex')
+    run = _last_run(tmp_path)
     assert run['is_successful'] is False
     assert run['summary'] == (
         '0 updated, 0 skipped, 1 failed, 1 already up to date'
@@ -1399,35 +1400,104 @@ def test_hook_run_right_after_another_does_nothing(tmp_path, auto_update_on):
     _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
     _, output = _run_hook(tmp_path, 'kiro', _make_skill_client())
     assert output == 'AWS agent skills updated: aws-s3.\n'
-    first = _last_run(tmp_path, 'kiro')
+    first = _last_run(tmp_path)
     _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
     client = _make_skill_client()
     rc, output = _run_hook(tmp_path, 'kiro', client)
     assert (rc, output) == (0, '')
     assert not client.get_latest_skill_version.called
-    assert _last_run(tmp_path, 'kiro') == first
+    assert _last_run(tmp_path) == first
 
 
-def test_hook_run_for_another_agent_is_not_skipped(tmp_path, auto_update_on):
+def test_hook_run_for_another_agent_skips_checked_root(
+    tmp_path, auto_update_on
+):
+    # The skills root was just checked through another agent's hook.
     _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
     _run_hook(tmp_path, 'kiro', _make_skill_client())
-    _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
-    _, output = _run_hook(tmp_path, 'codex', _make_skill_client())
-    assert output == 'AWS agent skills updated: aws-s3.\n'
+    client = _make_skill_client()
+    assert _run_hook(tmp_path, 'codex', client) == (0, '')
+    assert not client.get_latest_skill_version.called
 
 
-def test_hook_runs_again_once_last_run_is_old(tmp_path, auto_update_on):
-    _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
-    config = next(c for c in AGENT_CONFIGS if c.id == 'kiro')
+def _record_check(tmp_path, age, is_successful=True, agent_dir='.test-agent'):
     state.record_hook_run(
-        hooks.hook_config_path(config),
-        is_successful=True,
+        os.path.realpath(tmp_path / agent_dir / 'skills'),
+        is_successful=is_successful,
         summary='old',
-        timestamp=datetime.datetime.now(datetime.timezone.utc)
-        - datetime.timedelta(seconds=update_skill.HOOK_RECENT_RUN_SECONDS + 1),
+        timestamp=datetime.datetime.now(datetime.timezone.utc) - age,
     )
-    _, output = _run_hook(tmp_path, 'kiro', _make_skill_client())
-    assert output == 'AWS agent skills updated: aws-s3.\n'
+
+
+@pytest.mark.parametrize(
+    'age,is_successful,is_checked',
+    [
+        (update_skill.HOOK_CHECK_INTERVAL, True, True),
+        (
+            update_skill.HOOK_CHECK_INTERVAL - datetime.timedelta(minutes=1),
+            True,
+            False,
+        ),
+        (update_skill.HOOK_RETRY_INTERVAL, False, True),
+        (
+            update_skill.HOOK_RETRY_INTERVAL - datetime.timedelta(minutes=1),
+            False,
+            False,
+        ),
+        # A check recorded in the future means the clock moved back.
+        (-datetime.timedelta(days=7), True, True),
+    ],
+)
+def test_hook_checks_root_once_interval_passed(
+    tmp_path, auto_update_on, age, is_successful, is_checked
+):
+    _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
+    _record_check(tmp_path, age, is_successful)
+    client = _make_skill_client()
+    _, output = _run_hook(tmp_path, 'kiro', client)
+    assert client.get_latest_skill_version.called is is_checked
+    expected = 'AWS agent skills updated: aws-s3.\n' if is_checked else ''
+    assert output == expected
+
+
+def test_hook_checks_only_roots_that_are_due(tmp_path, auto_update_on):
+    _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
+    _install_skill_at_version(tmp_path, '.claude', 'aws-cdk', 'v1')
+    _record_check(tmp_path, datetime.timedelta(minutes=1))
+    _, output = _run_hook(tmp_path, 'claude-code', _make_skill_client())
+    assert json.loads(output)['systemMessage'] == (
+        'AWS agent skills updated: aws-cdk.'
+    )
+    # The root that wasn't due keeps its record and its old skill.
+    assert _last_run(tmp_path)['summary'] == 'old'
+    marker = _skill_dir(tmp_path) / SKILL_METADATA_FILENAME
+    assert json.loads(marker.read_text()) == {'version': 'v1'}
+    assert _last_run(tmp_path, '.claude')['is_successful'] is True
+
+
+def test_hook_records_failure_only_for_root_with_failed_skill(
+    tmp_path, auto_update_on
+):
+    _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
+    _install_skill_at_version(tmp_path, '.claude', 'aws-cdk', 'v1')
+    client = _make_skill_client(bad_checksum_skill='aws-s3')
+    _run_hook(tmp_path, 'claude-code', client)
+    assert _last_run(tmp_path)['is_successful'] is False
+    assert _last_run(tmp_path, '.claude')['is_successful'] is True
+
+
+def test_hook_records_failure_for_every_root_on_error(
+    tmp_path, auto_update_on
+):
+    _install_skill_at_version(tmp_path, '.test-agent', 'aws-s3', 'v1')
+    _install_skill_at_version(tmp_path, '.claude', 'aws-cdk', 'v1')
+    client = _make_skill_client()
+    client.get_latest_skill_version.side_effect = EndpointConnectionError(
+        endpoint_url='https://example.com'
+    )
+    _run_hook(tmp_path, 'claude-code', client)
+    assert _last_run(tmp_path)['is_successful'] is False
+    assert _last_run(tmp_path, '.claude')['is_successful'] is False
 
 
 def test_hook_does_nothing_while_another_run_holds_the_lock(