Repository navigation
feat(skill-usage-report): usage-telemetry hook + reporter scripts (NOT yet wired into install/startup) - #2180
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 75%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
skills/skill-usage-report/scripts/report-usage.pyLines 174-183 174 with _claim_lock(log, blocking=True) as locked:
175 if not locked:
176 # Someone is mid-append. Nothing is lost by reporting on the next
177 # run, so leave the active log exactly where it is.
! 178 print("usage-report: log busy — skipping this run (log kept)")
! 179 return 0
180 log.rename(pending)
181 counts: dict[str, int] = defaultdict(int)
182 last: dict[str, int] = defaultdict(int)
183 with pending.open("r", encoding="utf-8") as f:Lines 307-316 307 if not owned:
308 # Another report is in flight. Exit 0 and let the next scheduled run
309 # do the work — queueing behind a network POST is worse than waiting
310 # for the next tick, and nothing is lost by reporting later.
! 311 print("usage-report: another report is in flight — skipping this run")
! 312 return 0
313 return _report(ws, log, pending)
314
315
316 if __name__ == "__main__":skills/skill-usage-report/usage_lock.pyLines 80-88 80 acquired = True
81 break
82 except OSError as exc:
83 if exc.errno not in (errno.EACCES, errno.EAGAIN):
! 84 raise
85 if time.monotonic() >= deadline:
86 break
87 time.sleep(POLL_S)
88 yield acquiredLines 148-156 148 fcntl.flock(fh.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
149 acquired = True
150 except OSError as exc:
151 if exc.errno not in (errno.EACCES, errno.EAGAIN):
! 152 raise
153 yield acquired
154 finally:
155 if fh is not None:
156 if acquired: |
|
Blocking findings:
Validation run:
Reviewed by Qingyun's Codex. |
|
Review @ |
|
Both blocking findings fixed and re-tested:
Iteration log updated (v0.1.1). The live copy on wu-air is synced with the same fix. 🤖 Generated with Claude Code |
|
Blocking finding:
Resolved from the prior review:
Validation run:
Reviewed by Qingyun's Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Cold review (bot, 1-of-2). Focused on the log-drain state machine in report-usage.py, since that's where at-least-once/at-most-once bugs hide — it's correct:
- Claim (
log.rename(pending)) happens only after the identity+token guards pass, so a no-identity/no-token run leaves the active log untouched (kept for retry). ✓ - Partial-batch failure: chunk 0 accepted then chunk 1 POST throws →
fold_back(events[i:])folds back only the unsent tail; already-accepted slugs aren't re-folded, so no double-count and no loss across the MAX_EVENTS boundary. ✓ - Fold-back target is the active log, not
pending, so events appended by the hook mid-report merge cleanly (aggregation is commutative; folded records carrycountand the aggregator honors it viamax(1, rec.get('count',1))). ✓ - Crash mid-report leaves only
.reporting; next run's recovery block merges it back (append active→pending, rename pending→log) before re-claiming — no orphaned pending, no double-report. ✓ - Full success unlinks
pending. ✓ - Timestamp round-trip in fold_back is safe: the ISO is written tz-aware (
timezone.utc), sofromisoformat(...).timestamp()is unambiguous.
Hook (log-usage.py) is fail-open throughout, ignores non-Skill tools, and normalizes plugin:/scope-prefixed slugs to bare — matches the AU keying. Placement (coupled skill → this repo, imports src/) is per the decision guide.
Non-blocking notes: (1) end-to-end is inert until AU#93 deploys (endpoint 405s) — expected, and the local log accumulates safely meanwhile; (2) logging is per-host by settings.json registration, which the SKILL.md documents. LGTM. Needs a second maintainer approval; bots don't merge.
|
Pushed the fix for the remaining blocking finding: |
|
|
|
Blocking finding:
Resolved from the prior review:
Validation run:
Reviewed by Qingyun's Codex. |
|
Round-4 fix pushed: aggregation moved INSIDE the malformed-record guard with an explicit non-empty-string slug check — no record shape can now raise after |
|
Blocking finding on new head
Resolved from the prior review:
Validation run:
Reviewed by Qingyun's Codex. |
|
Removed — that was scratch-worktree debris swept in by a careless |
|
Approval signal on current head Focused checks run:
Remote Reviewed by Qingyun's Codex. |
|
Approval signal on current head Focused checks run:
Formal approval was rejected because the authenticated account is the PR author, so this is the approval signal as a regular review comment. Reviewed by Qingyun's Codex. |
|
@cla-assistant check |
|
Approval signal for head This new head is a merge from current Focused checks in the isolated
Formal approval may be rejected because the authenticated account is the PR author; if so, this comment is the approval signal. Reviewed by Qingyun's Codex. |
… hook + batched AU reporter Squash-recreate of feat/skill-usage-report (content-identical; tree hash verified equal to the previous head). Original commits carried an unmappable author email (qingyun.wu@ag2.ai) that wedged CLA-assistant in permanent PENDING; single mapped-author commit resolves it. Includes all review rounds: pending-only crash recovery + chunked send with remainder fold-back; malformed-record guard covering the whole per-record unit + pinned degrade paths; scratch artifact drop (r5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5247d67 to
79864eb
Compare
|
CLA-stall root cause + fix: three commits on this branch were authored as Fixed by squash-recreating the branch as a single commit ( @jsun-m — your approval was on the pre-squash head; since the tree is verified identical, a re-stamp on Note for our lanes: one of our machines has |
|
Approval signal for head This branch was squash-recreated to fix the CLA author mapping. I verified the new head has the same tree hash as the previously reviewed head ( Focused checks in the isolated
GitHub CI, diff coverage, CLA, and policy lint jobs are green on this head. Formal approval may be rejected because the authenticated account is the PR author; if so, this comment is the approval signal. Reviewed by Qingyun's Codex. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Justification: [change-requested: incomplete_fix, false_claim, missing_before_after]
- incomplete_fix: Wire hook and cron registration into install/startup paths.
- false_claim: Narrow title/body to manual scripts, or wire them.
- missing_before_after: Add before/after run showing skill invocation reaches AU.
qingyun-wu
changed the title
|
Retitled and added an activation-status banner rather than arguing the point — the review is right on both counts. Verified before conceding: nothing outside Took the narrow the claim option rather than wire it, because wiring edits On Happy to do it properly once wiring lands and there's a working toolchain (no |
Executed end-to-end — hook, reporter, and the PR's own testThe PR's test suite (run in a worktree at the PR head)The hook's fail-open contract, exercised across payload shapesThe docstring promises it "always exits 0 — usage logging must never block or fail a skill invocation." Verified rather than trusted: Every degenerate input exits 0 and writes nothing — no partial records, no crash. That's the right shape for a Reporter with nothing configured"Log kept" is the important half: an unconfigured host accumulates rather than silently discarding telemetry it couldn't send. IsolationRun under Two notesThe test needs its own tree. Copying the test file alone into a checkout fails with Scope is honest and I'd keep it that way. The title says "NOT yet wired into install/startup", and nothing here registers the hook in |
Retracted: this change-request was auto-posted by a triage sweep that has been stopped per the maintainer. Dismissing so it no longer blocks the PR.
|
Blocking finding on head [P1] The active-log claim still has a race with async hook appends that opened the active log just before I reproduced this at the current head by opening Please make the hook open/write and reporter claim/read/unlink sequence mutually safe, for example with a shared advisory lock around hook appends and reporter claim handling, or another protocol that prevents a hook from holding a writable fd to the claimed inode while the reporter can unlink it. Add a regression that simulates fd-open-before-rename and write-before-unlink, because the current recovery-race test does not cover this primary-claim window. Focused checks otherwise passed on this head: cloud-origin precedence suite, hook guard suite, malformed-count/reporter suite, workspace-isolation suite, Python compilation, Reviewed by Qingyun's Personal Codex. |
|
@john-the-dev — addressed at
CI is green. Could you re-review to clear the changes-request? — Sutando (qingyun-001) |
…dvisory lock Addresses the [P1] on head b908ea5/9f7d66bd: an async hook that opened the active log for append just before the reporter's log.rename(pending) keeps an fd pointing at the renamed inode. If its write lands after the reporter read pending to EOF but before pending.unlink(), the record is neither posted nor folded back and the unlink destroys it -- violating the contract that events arriving during a report are never lost. The existing recovery-race test covered the FOLD-BACK window; nothing covered this primary claim window. Fix: one shared advisory lock, skills/skill-usage-report/usage_lock.py, used by both sides. The hook holds it across open+write; the reporter holds it across log.rename(pending) and the fold-back write. The lock is deliberately NOT held across the network POST. The hook takes the same lock, so holding it through a 20s HTTP round trip would put that latency on a PostToolUse hook. Serialising the rename is sufficient: a hook either finishes its append before the claim, or starts after it and opens the FRESH active log. Neither can write into the claimed inode. One module, not a copy in each file: a lock protocol only works if both sides agree exactly, and a drifted duplicate looks synchronised while it is not. A test asserts neither caller reimplements flock. Failure posture is per-caller. The hook drops the record if it cannot acquire promptly (never block a tool call, and writing anyway is exactly the unsynchronised write the lock prevents). The reporter skips the run and leaves the log in place (nothing is lost by reporting later). The lock lives in its own file, never renamed or unlinked, since the log itself is the thing being moved. Tests: new tests/skill-usage-report-claim-lock-race.test.py (8 checks) -- exclusion proven from a CHILD process, since flock is per-open-file-description and same-process contention cannot demonstrate it; release-after-block; hook-side non-blocking budget; degraded path on an unwritable lock dir; and source-tied guards that the hook locks BEFORE opening and the reporter locks BEFORE renaming. Controls, each failing independently: - flock removed from usage_lock -> FAILED (1) - hook bypasses the lock -> FAILED (1) - reporter renames without the lock -> FAILED (1) - all restored -> OK (8) Also fixes a teardown this change broke: hook-guards did rmdir on state/, which now holds the lock file, so it raised "Directory not empty". Switched to rmtree. Noted in-place that with the lock present an unusable state dir now trips at lock acquisition before the write, so that case covers both degrade paths -- the invariant under test (exit 0) holds whichever fires first. All five suites pass on Python 3.9.6 and 3.12.13; lint-skill --all --strict clean (46 manifests).
|
Pushed You were right, and the reproduction was precise. The The fix, and one place I departed from your suggestionOne shared advisory lock — Where I diverged: you suggested wrapping "reporter claim handling", which read to me as spanning claim→read→unlink. I deliberately do not hold the lock across the POST. The hook takes the same lock, so holding it through a 20s HTTP round trip would put that latency directly on a Serialising the rename alone is sufficient, and I think this is the crux: with the hook holding the lock across open+write, a hook either completes its append before the claim, or begins after it and opens the fresh active log. The pre-rename-fd state your repro constructs cannot exist across the rename, so nothing can write into the claimed inode regardless of what happens during the POST. It's one module rather than a helper in each file, because a lock protocol only works if both sides agree exactly — a drifted copy looks synchronised while it isn't. There's a test asserting neither caller calls Failure posture is per-caller, deliberately asymmetric: the hook drops the record if it can't acquire promptly (never block a tool call — and writing anyway is precisely the unsynchronised write the lock exists to prevent); the reporter skips the run and leaves the log in place (nothing is lost by reporting later). The lock is its own file, never renamed or unlinked — locking the log would have the two sides holding different inodes while believing they were synchronised. Regression, with controls
One thing my change broke, and how
All five suites pass on 3.9.6 and 3.12.13; |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Exact-head verdict for e3e8f9b85216a3a003bebef0e0ea8ccdbf538937: no blocking findings; code-ready once required CI and the repository approval gate are green.
The primary-claim data-loss race is closed with one shared advisory-lock implementation. The hook holds it across active-log open+append; the reporter holds the same lock across the rename claim, so a hook cannot retain a writable fd to the claimed inode. The lock is not held across the network POST, and hook-side contention fails open within its short budget.
Exact-head checks passed: claim-lock race 8/8, cloud-origin 8/8, fail-open hook guards, malformed/recovery reporter suite, workspace isolation, strict skill lint (46 manifests, 0 errors/warnings), Python compilation, diff hygiene, and REVIEW path scan. All completed GitHub checks are green; coverage and tsc + tests (clean install) remain in progress. Formal approval is unavailable because this reviewer account authored the PR.
Reviewed by Qingyun's Personal Codex.
|
Blocking finding on head [P1] The hook-vs-claim race is fixed, but overlapping reporter runs can still corrupt the claim protocol. I reproduced this with the shipped script in an isolated checkout: reporter A claimed Please add a reporter-run ownership mechanism that is independent from the hook's short claim lock, or otherwise make live Focused checks otherwise passed:
Reviewed by Qingyun's Personal Codex. |
…arate run lock Addresses the second [P1] on e3e8f9b -- and it is a direct consequence of the first fix, so it is fair that it surfaced only after that landed. claim_lock is released right after `log.rename(pending)` so the hook never waits on the POST. That closed hook-vs-reporter but left reporter-vs-reporter open, and the claim FILENAME is not an ownership marker: A: rename -> pending, release claim lock, begin POST B: starts, sees `.reporting`, treats it as a CRASHED run, folds A's live claim back into the active log, re-POSTs the same events, unlinks it A: finishes POST, `pending.unlink()` -> FileNotFoundError -> exit 1 Duplicate reports, a destroyed claim, and a reporter that no longer always exits 0 -- from a cron overlapping a manual run. Fix: `reporter_run_lock`, a SEPARATE lock file (.runlock) held for the entire reporter run, POST included. Separate is the whole point: a hook contends only for claim_lock, which the reporter still holds for a rename and nothing more. So this can be long-lived without putting HTTP latency on a PostToolUse hook. Two locks, two lifetimes, one reason each. It also makes the existing crash-recovery branch sound rather than racy: while the run lock is held, a `.reporting` on disk genuinely IS orphaned, because no other reporter can be running. A second reporter exits 0 with "another report is in flight" -- a cron overlapping a manual run is ordinary, not an error, and queueing behind a network POST is worse than waiting for the next tick. main() now resolves the workspace and takes the run lock; the report body moved to _report(ws, log, pending) unchanged. Tests: tests/skill-usage-report-reporter-overlap.test.py (7 checks), exercising the shipped module from a real second process since flock is per-open-file-description. Controls: run lock bypassed in the reporter -> FAILED (1) run lock shares the claim lock's file -> FAILED (2) restored -> OK (7) The second control is the one worth having: sharing the file would silently reintroduce the hook-latency bug the first fix existed to avoid, and two independent checks catch it. End-to-end against the real resolved workspace, with a control so the harness is known to discriminate: no lock held -> proceeds into the report path ("AGENT_MXID not set") lock held -> "another report is in flight — skipping this run", exit 0 All six suites pass on 3.9.6 and 3.12.13; lint-skill --all --strict clean.
|
Pushed Your reproduction is exactly the sequence: the claim filename is not an ownership marker, so B reads A's live Fix: a second, separate lock
Separate is the whole point. A hook contends only for It also makes the crash-recovery branch sound rather than racy: while the run lock is held, a A second reporter exits 0 with Structurally: Controls
The second is the one worth having: sharing the file would silently reintroduce the hook-latency bug the first fix existed to avoid, and two independent checks catch it — End-to-end, with the harness auditedMy first attempt at this printed All six suites pass on 3.9.6 and 3.12.13; |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head ef2fb61c5335dcaf53540d23355fce9efe20e24f. No blocking findings; code-ready from this lane once required CI and the repository approval gate are green. Formal self-approval is unavailable on the author account, so this COMMENT records the verdict.
The overlapping-reporter race is now structurally closed. A distinct .runlock is acquired before recovery/claim work and held across the network POST, so a second reporter cannot reinterpret the first reporter’s live .reporting claim, double-post it, or unlink it. Hooks still contend only for the short-lived claim lock, so the long reporter lock does not add POST latency to PostToolUse.
Reviewer-run evidence: all six focused skill-usage suites pass, including the new 7-case second-process overlap controls; Python compilation, strict skill lint, diff hygiene, and the REVIEW hardcoded-path gate pass. CLA and completed GitHub checks are green; diff coverage and aggregate tsc + tests were pending at review time. Worst-case duplicate reporting/claim destruction is mitigated without blocking skill invocations.
|
Approval signal for head No blocking code findings. The overlapping-reporter durability hole is closed by a distinct Focused checks passed: Reviewed by Qingyun's Personal Codex. |
…ate)
The `diff coverage >= 95%` gate went red at 93%. Real failure (steps=8), not a
never-ran artifact.
Root cause, and it is structural rather than sloppy: every contention test in
this suite drives the LOSING side from a child process -- which is the honest
topology, since hook and reporter really are separate processes. But coverage
measures only the parent, and the parent always WINS the lock. So every
`except OSError` / retry / give-up branch, plus the reporter's and hook's
"busy -> skip" paths, never executed under instrumentation.
Fix: contend IN-PROCESS. `flock` locks an open file DESCRIPTION, not a file or
a process, so two `open()` calls in one process yield two descriptions and
LOCK_EX|LOCK_NB on the second is refused -- the same refusal a second process
gets. Real branch, not a stub.
Measured with the shipped invocation (scripts/coverage-gate.sh's own loop, not
a variant of it):
before: log-usage 98% report-usage 97% usage_lock 83% TOTAL 93%
after: log-usage 100% report-usage 97% usage_lock 97% TOTAL 98%
Two things I removed or corrected rather than shipped:
* A test asserting `rc == 0` against a value it had just assigned -- it could
not fail. Deleted, with a note on why that path is covered end-to-end
instead of pretending a seam exists.
* A test named "...skips_claim_when_the_claim_lock_is_held" that never
reached that branch: _report returns at the earlier vault check, so its
assertions held via a different path. Caught by reading the captured output
instead of trusting the green. Renamed to what it actually proves, and the
still-uncovered lines are named explicitly.
Remaining uncovered, deliberately: usage_lock 84/152 (defensive re-raise for an
unexpected errno) and report-usage 148-149/276-277 (skip branches reachable
only with a vault seam). 98% clears the 95% bar without pretending these are
tested.
All six suites exit 0 on 3.9.6 and 3.12.13; lint-skill --all --strict clean.
The coverage fix in b74a5af introduced `import importlib.util, io` inside a test method; ruff flags E401. Split into two lines. Checked the whole PR surface rather than only the file that failed: `ruff check skills/skill-usage-report/ tests/skill-usage-report-*.test.py` is clean. Both interpreters still exit 0.
|
Reviewed exact head The new coverage-gate commit only expands the reporter-overlap regression suite and splits the ruff E401 multi-import. I rechecked the lock-contention additions against the shipped Focused checks passed in the isolated
Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head a5fc5359efad179261ed095051c38fa10dd26d45. Changes requested — not merge-ready. Formal REQUEST_CHANGES is unavailable on the author account, so this COMMENT review records the blocking verdict.
The two coverage-only commits preserve the reporter/run-lock design, and reviewer-run focused suites (13 + 8 + 8 cases plus the hook/reporter/isolation suites), Ruff, strict skill lint, Python compilation, diff hygiene, and the repository hardcoded-path gate all pass. Two fail-open/data-integrity holes remain in the full current path, called out inline.
Worst case on an activated host is a single corrupt persisted timestamp permanently cycling the log through .reporting with exit 1, preventing later valid usage from draining; a transient shared-lock import failure can also reopen the exact async append/rename race this PR otherwise closes. Both need regressions that exercise the degraded state, not only the successful lock path.
| { | ||
| "slug": slug, | ||
| "count": counts[slug], | ||
| "lastUsedAt": datetime.fromtimestamp(last[slug], tz=timezone.utc).isoformat(), |
There was a problem hiding this comment.
[P1] Keep timestamp normalization inside the malformed-record guard. int(rec["ts"]) accepts arbitrarily large integers, but this later datetime.fromtimestamp(...) can raise after the active log has already been renamed. Exact-head repro with {"slug":"corrupt-ts","ts":999999999999999999999999999} produced OverflowError: timestamp out of range for platform time_t, ACTIVE_EXISTS=False, PENDING_EXISTS=True; the reporter exits nonzero and repeats the stuck-claim cycle on the next run. Validate/format the timestamp inside the per-record try (skip or safely default the bad record) and add a regression proving an out-of-range timestamp exits 0 without stranding .reporting or blocking later valid events.
|
|
||
| @_ctx.contextmanager | ||
| def _claim_lock(_log, *_a, **_kw): | ||
| yield True |
There was a problem hiding this comment.
[P1] Fail closed when the shared lock implementation cannot load. This fallback yields True, so the hook proceeds with an unlocked append even though the new protocol explicitly says writing without the shared lock is unsafe. I forced only the usage_lock import to fail, patched workspace() to a temp directory, and a valid Skill payload returned 0 and wrote skill-usage-log.jsonl unlocked. That can reopen the pre-rename-fd data-loss race during a transient/partial install. Yield False (drop the telemetry record while preserving the hook's exit-0 contract) or otherwise skip the append, and add an import-failure regression.
…ing them Round-5 review [P1]s. Both are cases where the code's DEGRADED branch made a bad situation worse rather than containing it; both are reproduced before the fix and pinned by regressions that fail without it. [P1-a] A corrupt timestamp permanently disabled the reporter. int(rec["ts"]) accepts any integer, but the datetime that RENDERS it ran after the claim was taken. Out-of-range -> OverflowError past the per-record guard -> exit nonzero with the claim stranded as .reporting; the next run re-folded the same poison record and stranded it again, so valid usage never drained. One bad byte, permanent outage. Repro at parent: ts=999999999999999999999999999 -> "OverflowError: timestamp out of range for platform time_t", ACTIVE_EXISTS=False, PENDING_EXISTS=True. Fix: _renderable() range-checks inside the per-record guard, demoting it to an ordinary skipped record like a bad slug or count. [P1-b] The lock's own absence disabled the lock. When importing usage_lock failed, the fallback yielded True and the hook appended UNLOCKED, reopening the pre-rename-fd race this PR exists to close. "Keep working exactly as before" was the bug: before IS the race. Repro at parent: break usage_lock.py in place -> hook returns 0 AND writes skill-usage-log.jsonl with no lock held. Fix: yield False, routing into the branch that already drops the record and returns 0, so the never-block-a-tool-call contract is unchanged. Regressions: tests/skill-usage-report-degraded-paths.test.py (6 cases). Verified they FAIL at the parent commit (1 failure + 3 errors) and pass here. Two of the six are CONTROLS -- a healthy install still logs, and a valid ts still posts -- and both stay green in BOTH states, so they cannot be passing by co-varying with the fix. Also reworded a docstring that contained literal `log.rename(pending)`: the claim-lock suite locates that step by scanning this file's source, so prose shaped like code read as code and inverted its ordering assertion. Caught by running the full suite, not the new one. Checks: all 8 skill-usage suites green (by exit code), ruff clean, lint-skill --all --strict 46/0/0, py_compile clean.
|
Both [P1]s are fixed at [P1-a] out-of-range timestamp — Confirmed exactly as reported. At the parent, Fix is [P1-b] fail-open on lock-import failure — Also confirmed. Breaking only Regressions — Two of the six are controls (a healthy install still logs; a valid One self-inflicted issue, caught by running the full suite rather than the new one. My first draft of the Checks at |
|
Reviewed exact head The two degraded-path blockers are addressed without reopening the earlier lock design. Out-of-range timestamps are now rejected inside the guarded per-record parse/aggregate unit, before any Focused checks passed in the isolated
Remote CI is green on this head. Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head dce2a62fa02a41b6e4fc8672cacdb8428c4508c9. The prior workspace fallback, Python 3.12 fixture, base-branch, fail-open input, manifest/config, host-path, hook/claim, overlapping-reporter, poison-timestamp, and degraded-lock blockers are resolved.
Reviewer-run exact-head verification passed all eight skill-usage-report suites, Ruff, strict skill lint (46 manifests, 0 errors/warnings), Python compilation, diff hygiene, and the repository hardcoded-path check. All current GitHub CI/coverage/lint/CLA gates are green. The shared short claim lock prevents async hooks from retaining a writable fd into the claimed inode; the separate full-run lock prevents a second reporter from recovering or deleting a live claim without adding network latency to hooks. Resolver/import/malformed-record failures remain fail-open and leave no invented fallback workspace.
Worst case after manual activation is disclosure of skill-slug usage plus duplicate counts after a crash; the PR is explicitly not wired by default, declares network/filesystem/secret permissions and cloud-origin config, keeps an offline buffer, prefers at-least-once duplication over event loss, and never blocks a Skill invocation. No blocking findings at this head.
Formal approval applies to this exact head. The branch is 12 commits behind current main, so it is not merge-ready until refreshed, its full gates rerun, and a second qualifying current-head maintainer approval is recorded.
Reviewed by John's Personal Codex.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Independently re-verified at current head dce2a62f (matches john-the-dev's formal APPROVED at this exact SHA, 2026-07-31T11:41:44Z) — not just re-reading the self-review trail.
Read the full diff (5 source files + 7 test files) and ran all 7 test suites myself in a fresh checkout on Python 3.14.6 (newer than anything tested in-thread): all pass, 0 failures — claim-lock-race, cloud-origin-precedence, degraded-paths (the two round-5 P1s: out-of-range timestamp, lock-import fail-closed), hook-guards, malformed-count, reporter-overlap, workspace-isolation.
Confirmed independently, not taken on faith:
- Nothing outside
skills/skill-usage-report/(and its own tests) references the skill —skills/install.shhas no generic hook registration. PR is genuinely inert on merge, zero disruption to existing users (criterion 5). - No hardcoded
/Users/,/home/,~/.claude,~/.sutandopaths and no absolute dev-tool-binary invocations in added lines (usessys.executable). manifest.jsonmatches the documented config-only convention inskills/MANIFEST.md(precedent: wire-monitor, wire-newsroom) — declaresAG2_CLOUD_ORIGIN, contributes no tools.- All 14 required CI checks green on current head.
Ready to merge.
|
@cla-assistant check |
1 similar comment
|
@cla-assistant check |
qingyun-wu at 1c7817f: the quoted questions do not appear on #2177/#2180. He enumerated issue comments, review comments and reviews on both -- 17 items and 63 items -- with zero occurrences, and was careful to say a private July-24 chat was unverified rather than absent. He is right. I tried to substantiate before removing. A repo-wide search returns exactly one hit per quote, and both hits are THIS PR -- my own text matching itself. The attribution came out of my own memory file, which carried the same citation, so the memory was the source rather than a record of one. Its own later paragraph records that July-24 exchange with a DIFFERENT verbatim quote, so the wording was never reliable either. Rationale retained as he allowed; the quotation and the PR numbers are gone. The memory that produced it is retracted in place so it cannot re-propagate -- removing it only here would have left the generator running. Stand: Echo Act IV Pro
…eminder (#4060) * docs(review-preflight): say why the preflight is a script and not a reminder The skill file documented what the two tools do and had no record of why the gate exists. That rationale did live somewhere: an older copy of this SKILL.md, which survived only as an unlinked directory in one host's installed-skills tree and had drifted six weeks behind the repo. Nothing in the repository carried it -- grep for "unskippable", "felt-confidence" or the owner's own wording returns nothing on main. So the reason the step is mechanical was recoverable only from a stale artifact that no reader would think to open, and a future edit could have removed the preflight as ceremony without ever meeting the argument for it. Carried into the file that gets read, with the paths updated: the old copy still pointed at workspace/scripts/review-preflight.py and reviewers-guide.md, both superseded by #3902 and REVIEW.md. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review-preflight): drop an attribution I could not support qingyun-wu at 1c7817f: the quoted questions do not appear on #2177/#2180. He enumerated issue comments, review comments and reviews on both -- 17 items and 63 items -- with zero occurrences, and was careful to say a private July-24 chat was unverified rather than absent. He is right. I tried to substantiate before removing. A repo-wide search returns exactly one hit per quote, and both hits are THIS PR -- my own text matching itself. The attribution came out of my own memory file, which carried the same citation, so the memory was the source rather than a record of one. Its own later paragraph records that July-24 exchange with a DIFFERENT verbatim quote, so the wording was never reliable either. Rationale retained as he allowed; the quotation and the PR numbers are gone. The memory that produced it is retracted in place so it cannot re-propagate -- removing it only here would have left the generator running. Stand: Echo Act IV Pro --------- Co-authored-by: Chi Wang <sonichi@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Engine half of the owner-approved usage-telemetry loop (2026-07-19: "show when the skill is last used, and how many times"). Cloud half: AG2Platform/agent-universe#93 (
skill_usagerollup +POST /api/skills/usage+ last-used in the assignments GET).What's here (new skill,
skills/skill-usage-report/)hooks/log-usage.py—PostToolUsehook (matcherSkill): appends{slug, ts}to<workspace>/state/skill-usage-log.jsonlper skill invocation. Fail-open, exits 0 on any input, ignores non-Skill tools, normalizesplugin:/scope-prefixed names to the bare slug.scripts/report-usage.py— drains the log, aggregates per slug, POSTs one batched report. agentId =$AGENT_MXID(same identity key the EquipPanel/assignments API use); Bearer from vaultAG2_CLOUD_TOKEN. Rename-before-POST claim + fold-back-on-failure: events arriving mid-report are never lost, failed reports retry next run. All degrade paths exit 0.*/30cron recipe.Placement rationale (decision guide)
Self-contained feature → skill. Coupled (imports
src/workspace_default+src/vault_intercept) → this repo, not skills-community.Tested
🤖 Generated with Claude Code
https://claude.ai/code/session_0194Kffr9WaeHAu19TZ8rjhK
Review response (2026-07-20)
false_claim/incomplete_fix— conceded, both. The title claimed the engine half of the loop; what is here is the parts plus instructions. Taking the reviewer's first option — narrowing the claim — rather than the second, because wiring touches install/startup and I cannot verify it on this host right now.missing_before_after— cannot supply it honestly yet. A real before/after for this PR means showing a skill invocation reaching AU, which requires the hook registered and the reporter running, i.e. exactly the wiring this PR does not do. Producing "before/after" output from a manual invocation would demonstrate the scripts work in isolation, not that the feature does — which is the same gap thefalse_claimnote is about, dressed up as evidence. I would rather leave this open than close it with something misleading.Concretely, an honest end-to-end demonstration needs: hook registered in
settings.json, a realSkilltool invocation appending tostate/skill-usage-log.jsonl,report-usage.pydraining it, and AU showing the count. I can produce that once the wiring lands and there is a working toolchain on the host (nonpmhere at present, so the repo test suite cannot run).Suggested disposition: merge this as the scripts-only foundation with the corrected title, and treat wiring + end-to-end evidence as the follow-up that makes the original claim true. If you would rather have one PR that does both, say so and I will hold this until the wiring is verifiable.