feat(telemetry): print a post-command notice on first run - #2156
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice, clean refactor. The move to lazy first‑run persistence in src/index.ts (with set() updating the cache before telemetry emits) means getResourceAttributes() on shutdown still sees the freshly persisted installationId, so telemetry keeps working within the first-run session. The printFirstRunNotice unit test is appropriately scoped and doesn't over‑mock.
One small behavioral note (not blocking):
src/handlers/config/handler.tsx—coerceValueswitches ontypeof currentagainstDEFAULT_GLOBAL_CONFIG. SinceDEFAULT_GLOBAL_CONFIG.installationIdis nowundefined, runningagentcore config installationId <uuid>will fall through to thedefaultbranch and throwUnsupported target type "undefined". Previously the default was a random UUID string, so this path worked. It's a rare command and there was no test covering it before, but if you want to preserve the ability to overrideinstallationIdfrom the CLI, you'd need to either treatundefinedcurrent-values as strings incoerceValue, or key the switch on the schema rather than the default value. Otherwise consider it intentional thatinstallationIdis now effectively read‑only viaagentcore config(users can still edit~/.agentcore/config.jsondirectly).
Also worth considering (optional): if globalConfigAccessor.set() fails on first run, the notice will be re‑shown on every subsequent invocation and telemetry will keep failing schema validation (silently) for the whole session. Current behavior is best‑effort with a warning log, which seems acceptable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2156 +/- ##
=========================================
Coverage 97.25% 97.25%
=========================================
Files 507 508 +1
Lines 33881 33899 +18
=========================================
+ Hits 32951 32969 +18
Misses 930 930 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8254946 to
bc4bef2
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| const configFileData = await this.readConfigFile(); | ||
|
|
||
| // if no installationId is present, generate one and merge it into the file data | ||
| if (!configFileData.installationId) { |
There was a problem hiding this comment.
Would it be simpler to return isFirstRun in the output here? Would that avoid having to make many of these other changes?
There was a problem hiding this comment.
I considered this approach, (and actually implemented it initially), but I didn't like the behavior of get writing to the config as a side effect since a caller would expect a strict-read here.
However, I can see the argument that from the perspective of the caller, it is a strict-read still.
Let me swap to the simpler approach and if we see issues with the side-effect, we can revisit.
bc4bef2 to
37f2104
Compare
|
Claude Security Review: no high-confidence findings. (run) |
37f2104 to
204338e
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| * Writes the telemetry-collection notice to the given stream on the first run of | ||
| * the CLI, unless telemetry is already disabled. | ||
| */ | ||
| export function printFirstRunNotice( |
There was a problem hiding this comment.
Can we shift this function under the handler file since it's only used by the handler?
There was a problem hiding this comment.
I was hoping to keep the root level entrypoint clean, and splitting it out made it easier/more natural to add unit tests, but I agree exporting this from telemetry module for a single consumer adds some indirection that shouldn't be necessary.
|
Merging since @AlexanderRichey comment is addressed and received another review. Can address any remaining comments as follow-ups. |
Problem
We are missing the post-first run telemetry notice on the refactor branch.
Solution
Verification
notice that it only printed the first time.