docs(agents): record the three app metrics decisions that look wrong - #191
Merged
Conversation
Adds items 5-7 to "Intentional decisions that look wrong (read before 'fixing')" and updates the section's framing sentence. 5. `civitai app metrics` calls tRPC, not REST — because there is no REST route to call. Owner analytics exist only as `blocks.getMyAppAnalytics`; there is no /api/v1 equivalent. The command therefore resolves slug -> appBlockId via the existing REST GET /api/v1/blocks/submissions and then issues the non-batched tRPC GET, reusing the authedDo + result.data.json unwrap pattern GetForgejoCloneInfo established. Documented so nobody "fixes" it into a REST call that does not exist. 6. `notOwned` is a cross-repo contract. The proc sits behind the `appBlocksAuthor` flag and answers a non-entitled caller with HTTP 200 and every counter zeroed, so a renderer that ignores it prints a plausible empty dashboard for a permission failure. The human view refuses to render on `notOwned`; `--json` deliberately passes it through and still exits 0, so scripts must branch on it themselves. Whoever changes the payload server-side has to keep the field. 7. The exit-code contract is pinned by errors.Is, never by message text. The sentinels carry no visible text (Tag/TagStatus preserve Error() byte-for- byte), so a message assertion says nothing about the exit code. Measured on the metrics PR: stripping the classification while leaving every message identical left the ENTIRE suite green, and the README's 403 -> exit 3 / not-found -> exit 4 promise was unpinned. Generalised to every command that claims an exit code. Sentinel names verified against the code, not paraphrased: the HTTP kinds are `civitai.ErrUnauthorized`/`ErrNotFound` in pkg/civitai/errkind.go, but the usage sentinel is `cmd.ErrUsage` in internal/cmd/usage_error.go (there is no `civitai.ErrUsage`) — the item says so explicitly. DEPENDS ON #190: internal/cmd/app_metrics.go and internal/appapi/analytics.go exist only on `feat/app-metrics` and are NOT on main. Merge this AFTER #190 or AGENTS.md will describe a command the tree does not have. `make ci` green: tidy + vet clean, 16/16 packages ok, 0 FAIL; `gofmt -s -l` prints nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01858ymA3tEJQi83435u7npi
ZacxDev
added a commit
that referenced
this pull request
Aug 3, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01858ymA3tEJQi83435u7npi
ZacxDev
added a commit
that referenced
this pull request
Aug 3, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01858ymA3tEJQi83435u7npi
ZacxDev
added a commit
that referenced
this pull request
Aug 4, 2026
* docs: handoff for app-analytics CLI command + two platform fixes Captures the state of #190, civitai/civitai#3557 and #3561, the stale-integration-gate blocker, the measured per-app analytics, ranked follow-ups, and the session's reusable lessons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01858ymA3tEJQi83435u7npi * docs: re-gate cleared the stale-gate blocker; #3557 prettier fixed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01858ymA3tEJQi83435u7npi * docs: record the three docs PRs and the #191-after-#190 merge constraint Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01858ymA3tEJQi83435u7npi * docs(handoff): record the shipped outcomes so the doc stops reading as a to-do list The app-analytics handoff has been sitting on this branch, unmerged, describing three open PRs and a re-gate in flight. All of that shipped — 8 PRs across both repos, plus follow-ups #1, #2 and #4 — so as written the doc's most prominent content is a set of live-sounding directives for work that is done. A stranded doc that is also stale is worse than no doc. Changes: - A RESOLVED banner up top with the full merged-PR table, replacing "nothing merged". - Neutralised the "Do not merge until that re-gate reports" imperative, and recorded that the rebase it anticipated WAS needed, for a different reason: #3566 landed later and edited the same `detail: {}` object, turning #3561 CONFLICTING after its gate had passed. - Struck follow-ups #1, #2 and #4 with what actually happened, including the two places this doc was WRONG: * #1's entry missed a second proc with the identical live defect — `getMyForgejoCloneInfo`, which `civitai app pull` drives. It was found by an audit, not by the list, which is worth knowing about ranked follow-up lists in general: the list is not a survey. * #2's suggested fix (reuse `humaniseScopeEndpoint`) would have shipped a bug. Measured against the real function it returns '(no workflow id)' for `workflow:submit` and '' for `user-settings:write`, because it is the per-ROW labeller and an aggregate bucket has no `detail`. - Recorded #1's scope decision with the prod evidence that later confirmed it: 331 live tokens unblocked, 30 of which lack bit 26 — so copying the nearest precedent (AppBlocksDevTunnel) would have left those 30 still 403ing. Plus the measurement trap: `(mask & Full) = Full` is also true when `mask == Full` and reports 145 false hits; the strict-superset form needs `AND mask <> Full`. - New "Still open — start here" section: the CI `component`-tier gap (three PRs shipped browser tests that have never run on a canonical browser), the stale-node_modules trap that silently removes ~1,126 tests, the unverified `addCollaborator` downgrade lead, and a note that `installs: 0` should be assumed broken until a positive control exists. - "What actually caught the bugs": across 12 adversarial audit rounds every fix round found a defect in the previous fix, and the mechanical gate caught none of them — the suite and typecheck were green at every tip. Doc-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(handoff): retract the "CI does not run the component project" claim — it was false The "Still open" section asserted that CI does not run the `component` (browser) project at all, and used that to frame the browser tests shipped today as having no CI coverage. **That was wrong.** The `preview / component-tests` external check runs exactly that project. Recording HOW the error happened, because the mechanism is more useful than the fact: every status query written during the session filtered for `Unit tests|Typecheck|ESLint|event-engine`, so `preview / component-tests` was never in a result set — and its absence from those results was read as evidence it did not exist. The evidence was selected and then the selection was treated as the finding. The claim was then repeated in three PR comments and this doc without ever being checked directly. What the check actually shows on #3574, the PR that shipped 7 browser tests: 075519d380 (before the audit-fix round) success 8e75826616 (first fix round) FAILURE 928273e4dd (second fix round) FAILURE It is report-only, which is why the merge was not blocked. Cutting the other way: PR 3591 PASSES component-tests on a base that contains the #3574 merge, while PR 3594 fails on the same base — green for some PRs, red for others, which looks like flakiness or content-dependence rather than a defect #3574 introduced. Genuinely unresolved. Also recorded: this cannot be settled from a NixOS host. The full-project component run does not complete locally either WITH the merge (crashes, "Browser connection was closed") or WITHOUT it (times out at 25 minutes) — measured both ways specifically to check whether the local failure was attributable to the change. It is not. Single-file runs pass, cold cache included, so a local run can validate one file and says nothing about the suite. The preview pipeline's logs are the only authority. And one genuinely stale thing found while investigating, left as an actionable note: `lint.yml`'s unit job excludes browser tests on the grounds that they "carry the cold-optimizeDeps flake documented at vitest.config.mts:98-124" — but that section documents the FIX, and a cold-cache single-file run passes. The stated reason no longer holds. Doc-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(handoff): add the followups handoff, with the component-tests root cause Second doc on this PR. The predecessor records the original session; this one records where the stream stands now and carries one fully-diagnosed open bug. The headline: `preview / component-tests` went success -> failure exactly at #3574's audit-fix round, and the root cause is mine. The 'Runs (range)' stat's tooltip begins 'Generations run through your app...', my test asserts getByText('Generations'), and getByText is substring + case-insensitive — so the locator resolves to 2 elements whenever that hover-only tooltip is mounted. civitai#3593 established that vitest browser mode shares ONE page across every .browser.test.tsx file, so a leftover pointer position from an earlier file can have it open at mount: fails in a full-suite run, never in a single-file run, which is exactly the observed pattern. Confirmed by computation that the PREVIOUS label ('Generation submits') did NOT substring-match that tooltip, so the vocabulary rename is what introduced it. Doc includes the verbatim fix to apply, the eliminations (cold-optimizeDeps flake is fixed; PR 3591 passes on a base containing the merge, so the suite is not universally broken; a local repro cannot settle it because the full-project run fails on this host with AND without the change), and the environment traps that produced false greens all session. Doc-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(handoff): mark the component-tests investigation resolved (civitai#3606) Records the fix, the proof matrix, and the reproduction trap: the tooltip target is the 14px IconInfoCircle, not the label — hovering the label does NOT mount it, so a probe without its own positive control reads as 'hypothesis wrong' and would have shipped an unproven fix. Also flags what is still unconfirmed: preview/component-tests had not reported on #3606 at merge time, so the next PR's result on that check is the confirmation to read. Doc-only. * docs(handoff): sync the header with the fix, and record the pins-vs-published blocker Goal/state lines still described the component test as outstanding. Also records why cli#193 is BLOCKED: pins-vs-published is red on main too (npm published past the scaffold pins), it has nothing to do with these docs, and the one-command fix unblocks it while fixing a real defect. Doc-only. * docs(handoff): record the pins blocker as fixed by cli#194 The `pins-vs-published` blocker described here is resolved — cli#194 (189d5a4) bumped @civitai/app-sdk ^0.28.0 -> ^0.30.0 and @civitai/blocks-react ^0.37.0 -> ^0.38.0 via the repo's own bump-pins command. Three corrections to what this doc told the next session: - The bumper rewrites scaffold_test.go's assertions ITSELF, along with the package.json.tmpl and README.md.tmpl literals. The doc's "then update the matching assertions in scaffold_test.go" implied a hand-edit that would have been redundant. - The blocker is RECURRING, not a one-off: it fires whenever npm publishes a new @civitai/* minor, on any open PR regardless of content. Noted, with a pointer to check why bump-scaffold-pins.yml did not open the bump PR on its own. - Recorded the one thing the bumper gets slightly wrong (a README prose line that overstates the minimum SDK version post-bump), so it is not rediscovered. Also renumbers the ranked list and strikes the two completed items. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vvMdxcMKJP9ripQLEiPm9 * docs(handoff): correct the bump-pins lead — cron latency, not a broken workflow My previous commit told the next session to "check why bump-scaffold-pins.yml did not open the bump PR itself". Measured it instead of leaving the hint: the workflow is fine and the framing was wrong. The scheduled run fired Mon 2026-08-03 10:51 UTC and correctly no-op'd. Both publishes landed LATER THE SAME DAY — @civitai/blocks-react 0.38.0 at 20:15 UTC, @civitai/app-sdk 0.30.0 at 22:13 UTC (npm registry `time` field). A successful no-op run and a broken detector look identical from the run list alone; the publish timestamps are the signal that separates them. The real exposure is the weekly cron (`17 7 * * 1`): a required check can sit red on main and every open PR for up to ~7 days after any @civitai/* minor. Records the two consequences — the fixed remedy (or `gh workflow run`), and that the lever is cron frequency, flagged as ask-first per AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vvMdxcMKJP9ripQLEiPm9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds items 5–7 to
AGENTS.md→ "Intentional decisions that look wrong (read before 'fixing')", and updates the section's framing sentence (items 1–3 mirrors, 4 a deliberate non-mirror, 5–7 the analytics read path).5. It calls tRPC, not REST — because there is no REST route to call
Owner analytics exist only as
blocks.getMyAppAnalytics(civitai/civitai → src/server/routers/blocks.router.ts); there is no/api/v1equivalent. So the command resolves<slug>→appBlockIdthrough the existing RESTGET /api/v1/blocks/submissions, then issues the non-batched tRPC GET (?input={"json":{…}}, unwrappingresult.data.json) reusing theauthedDo+ envelope-unwrap patternGetForgejoCloneInfoalready established. Written down so nobody "fixes" the two-hop shape into a REST call that does not exist.6.
notOwnedis a cross-repo contractThe proc sits behind the
appBlocksAuthorfeature flag and answers a caller who isn't entitled (or doesn't own the app) with HTTP 200 and every counter zeroed, not an error — so a renderer that ignores the field prints a plausible empty dashboard for what is really a permission failure.runAppMetricsrefuses to render when it's true. Whoever changes that payload server-side has to keep the field.--jsondeliberately passes the payload through and still exits 0, so scripts must branch onnotOwnedthemselves.7. The exit-code contract needs
errors.Is, not message assertionsThe classification sentinels carry no visible text —
Tag/TagStatusattach them whileError()stays byte-for-byte identical (pkg/civitai/errkind.go) — so a message assertion says nothing about the exit code. Measured on #190: stripping the classification while leaving every message identical left the entire suite green, and the README's 403 → exit 3 / not-found → exit 4 promise was unpinned. Generalised in the text to any command claiming an exit code.One correction to the brief I was given, made from the code rather than paraphrased: there is no
civitai.ErrUsage. The HTTP kinds (ErrUnauthorized,ErrNotFound) live inpkg/civitai/errkind.go; the usage sentinel isErrUsageininternal/cmd/usage_error.go. The item states the split explicitly so the guidance is actionable rather than a name that won't compile.Verification
make cigreen in a clean worktree offorigin/main:go mod tidyandgo vetsilent, 16/16 packagesok, zeroFAIL, build succeeded.gofmt -s -l .prints nothing. Docs-only change, so no behaviour is claimed to be verified beyond the gate.🤖 Generated with Claude Code
https://claude.ai/code/session_01858ymA3tEJQi83435u7npi