Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc232fff3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
habdelra
reviewed
Sep 21, 2026
habdelra
left a comment
habdelra
left a comment
Contributor
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed the check itself — what it extracts, what it exempts, what it walks, and whether it actually runs — against base's source on the head commit. I did not re-review the stacked date-fns declaration.
No blocking issues. Three asks, all inside the new script, all about it claiming slightly more than it delivers; the eslint-config one is the one I would fix before merge, since it is the justification the next author reads before proposing the script's removal.
One thing the description raises that is worth settling: treating peerDependencies as declared is correct, not a hole. pnpm records base's peer entries as importer dependencies in the lockfile (date-fns, ember-provide-consume-context, ember-source, lodash-es all resolve there), so they are linked into packages/base/node_modules and the check's premise holds for them.
Recommendations:
- Correct the eslint paragraph in the script header — base does have an eslint config, and the real reason is the resolver early-return. See the thread on lines 7–8.
- Add
.gjstoSOURCE_EXT, matchingexecutableExtensionsand base's own eslint overrides. See the thread onSOURCE_EXT. - Narrow (or tighten) the "anchored … so prose is not mistaken for one" claim — the match crosses newlines, and
DYNAMIC_IMPORTis unanchored. Repro in the thread onFROM_IMPORT.
No red checks; Lint is green on the head commit, which also confirms the wiring — base's lint fans out with concurrently "pnpm:lint:*(!fix)", so the new script is picked up without a CI change.
Adjacent, out of scope: experiments-realm and software-factory are card source with their own manifests and carry the identical exposure, with no equivalent check. The script is parameterized by nothing but its own location, so hoisting it to a shared helper is a plausible follow-up — not asked of this PR.
Generated by Claude Code
Nothing asked this question. TypeScript resolves modules by walking up to the workspace root, where a package hoisted for some other consumer resolves fine, so it checks a module exists and never that this package declared it. eslint's `import/no-extraneous-dependencies` would ask it, but it is not part of `import/recommended`, nothing in the repo enables it, and `eslint-plugin-import` is not one of base's devDependencies. An undeclared import works until something compiles base's source against base's own dependencies, because pnpm links only declared packages. Two reached main that way: `ember-provide-consume-context`, declared at a range that installed a second copy and broke context lookup, and `date-fns`, never declared at all. Both were found by bundling base into the host, not by tooling. Removing either declaration makes this check fail and name the importing files. A script rather than the eslint rule because base is card source, and four of its import idioms are not npm packages: `@ember/*` ships inside ember-source, `@cardstack/boxel-host/*` resolves through the virtual network's shim and cannot be declared without a cycle, `@cardstack/base/*` is self-referential, and card source may import from a URL. The rule would report well over a hundred violations, nearly all legitimate. The exemption list carries the reasoning for each, since "it resolves at runtime" is what hid both bugs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file went in unformatted: the prettier run I used could not load `prettier-plugin-ember-template-tag` and failed without writing, and I read the failure as noise rather than as the format step not having run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three corrections from review.
The header said base has no eslint config. It has a substantial one; the
measured reason the rule cannot do this is that it early-returns on any
specifier it cannot resolve, and under pnpm an undeclared package is
exactly the unresolvable case. The wrong reason is worse than none here:
it invites the next author to delete the script and configure a rule that
silently passes everything.
`.gjs` was missing from the scanned extensions, though it is in
`executableExtensions` and base's own eslint config already provisions for
it. No `.gjs` files exist in base today, so this closed a gap rather than
a miss.
The claim that the line anchor kept prose out was false: the gap before
`from` crosses newlines, so `export class A {}` followed by a comment
matched whatever quoted text came next, and `import()` inside a template
literal matched too. Comments and template-literal bodies are now blanked
before scanning — as a scan rather than a regex, because `//` inside a
string starts no comment and base imports from URLs, so a naive strip
would truncate a real specifier. Angle brackets are excluded from the gap
for Glimmer templates, which are not JS strings and survive the blanking.
Four false positives reproduced from review now extract nothing, while
single-line, multi-line, side-effect, dynamic, re-export and URL imports
all still extract. Removing either historical declaration still fails the
check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
backspace
force-pushed
the
cs-13057-catch-base-importing-a-package-it-does-not-declare
branch
from
September 21, 2026 15:04
6e19b1e to
620cbcf
Compare
habdelra
approved these changes
Sep 21, 2026
Contributor
Observability diff (vs staging)Show diffdiff --git a/tmp/remote-canon.pmhXYx/dashboards/boxel-status/actions-queue.json b/tmp/committed-canon.A9bTxe/dashboards/boxel-status/actions-queue.json
index db7fb3b..c9f773b 100644
--- a/tmp/remote-canon.pmhXYx/dashboards/boxel-status/actions-queue.json
+++ b/tmp/committed-canon.A9bTxe/dashboards/boxel-status/actions-queue.json
@@ -9,20 +9,7 @@
},
"spec": {
"annotations": {
- "list": [
- {
- "builtIn": 1,
- "datasource": {
- "type": "grafana",
- "uid": "-- Grafana --"
- },
- "enable": true,
- "hide": true,
- "iconColor": "rgba(0, 211, 255, 1)",
- "name": "Annotations & Alerts",
- "type": "dashboard"
- }
- ]
+ "list": []
},
"description": "GitHub Actions queue depth, wait times and runner consumption, sampled from the Actions REST API and shipped as JSON log lines on channel boxel:actions-queue.",
"editable": true,
@@ -336,262 +323,6 @@
"title": "Longest current wait",
"type": "stat"
},
- {
- "collapsed": false,
- "gridPos": {
- "h": 1,
- "w": 24,
- "x": 0,
- "y": 9
- },
- "id": 40,
- "panels": [],
- "title": "Right now",
- "type": "row"
- },
- {
- "datasource": {
- "type": "loki",
- "uid": "loki"
- },
- "description": "A point-in-time count, unlike the timeseries above — a branch whose jobs have finished disappears from this table instead of leaving a line behind.",
- "fieldConfig": {
- "defaults": {
- "custom": {}
- },
- "overrides": [
- {
- "matcher": {
- "id": "byName",
- "options": "branch"
- },
- "properties": [
- {
- "id": "links",
- "value": [
- {
- "targetBlank": true,
- "title": "Actions runs for this branch",
- "url": "https://github.com/cardstack/boxel/actions?query=branch%3A${__value.raw}"
- }
- ]
- }
- ]
- }
- ]
- },
- "gridPos": {
- "h": 8,
- "w": 8,
- "x": 0,
- "y": 10
- },
- "id": 41,
- "interval": "5m",
- "targets": [
- {
- "datasource": {
- "type": "loki",
- "uid": "loki"
- },
- "editorMode": "code",
- "expr": "last_over_time({service=\"actions-collector\", env=\"$env\"} |= \"boxel:actions-queue\" | json | line_format \"{{ if .log }}{{ .log }}{{ else }}{{ __line__ }}{{ end }}\" | json | channel=\"boxel:actions-queue\" | event_type=\"group\" | dimension=\"branch\" | unwrap queued [5m]) by (key) > 0",
- "queryType": "instant",
- "refId": "A"
- }
- ],
- "title": "Queued now, by branch",
- "transformations": [
- {
- "id": "organize",
- "options": {
- "excludeByName": {
- "Time": true
- },
- "renameByName": {
- "Value": "jobs",
- "Value #A": "jobs",
- "key": "branch"
- }
- }
- },
- {
- "id": "sortBy",
- "options": {
- "fields": {},
- "sort": [
- {
- "desc": true,
- "field": "jobs"
- }
- ]
- }
- }
- ],
- "type": "table"
- },
- {
- "datasource": {
- "type": "loki",
- "uid": "loki"
- },
- "description": "Which branches are occupying the pool at this moment.",
- "fieldConfig": {
- "defaults": {
- "custom": {}
- },
- "overrides": [
- {
- "matcher": {
- "id": "byName",
- "options": "branch"
- },
- "properties": [
- {
- "id": "links",
- "value": [
- {
- "targetBlank": true,
- "title": "Actions runs for this branch",
- "url": "https://github.com/cardstack/boxel/actions?query=branch%3A${__value.raw}"
- }
- ]
- }
- ]
- }
- ]
- },
- "gridPos": {
- "h": 8,
- "w": 8,
- "x": 8,
- "y": 10
- },
- "id": 42,
- "interval": "5m",
- "targets": [
- {
- "datasource": {
- "type": "loki",
- "uid": "loki"
- },
- "editorMode": "code",
- "expr": "last_over_time({service=\"actions-collector\", env=\"$env\"} |= \"boxel:actions-queue\" | json | line_format \"{{ if .log }}{{ .log }}{{ else }}{{ __line__ }}{{ end }}\" | json | channel=\"boxel:actions-queue\" | event_type=\"group\" | dimension=\"branch\" | unwrap running [5m]) by (key) > 0",
- "queryType": "instant",
- "refId": "A"
- }
- ],
- "title": "Runners held now, by branch",
- "transformations": [
- {
- "id": "organize",
- "options": {
- "excludeByName": {
- "Time": true
- },
- "renameByName": {
- "Value": "jobs",
- "Value #A": "jobs",
- "key": "branch"
- }
- }
- },
- {
- "id": "sortBy",
- "options": {
- "fields": {},
- "sort": [
- {
- "desc": true,
- "field": "jobs"
- }
- ]
- }
- }
- ],
- "type": "table"
- },
- {
- "datasource": {
- "type": "loki",
- "uid": "loki"
- },
- "description": "The same instant count grouped by whoever triggered the run.",
- "fieldConfig": {
- "defaults": {
- "custom": {}
- },
- "overrides": [
- {
- "matcher": {
- "id": "byName",
- "options": "author"
- },
- "properties": [
- {
- "id": "links",
- "value": [
- {
- "targetBlank": true,
- "title": "GitHub profile",
- "url": "https://github.com/${__value.raw}"
- }
- ]
- }
- ]
- }
- ]
- },
- "gridPos": {
- "h": 8,
- "w": 8,
- "x": 16,
- "y": 10
- },
- "id": 43,
- "interval": "5m",
- "targets": [
- {
- "datasource": {
- "type": "loki",
- "uid": "loki"
- },
- "editorMode": "code",
- "expr": "last_over_time({service=\"actions-collector\", env=\"$env\"} |= \"boxel:actions-queue\" | json | line_format \"{{ if .log }}{{ .log }}{{ else }}{{ __line__ }}{{ end }}\" | json | channel=\"boxel:actions-queue\" | event_type=\"group\" | dimension=\"actor\" | unwrap queued [5m]) by (key) > 0",
- "queryType": "instant",
- "refId": "A"
- }
- ],
- "title": "Queued now, by author",
- "transformations": [
- {
- "id": "organize",
- "options": {
- "excludeByName": {
- "Time": true
- },
- "renameByName": {
- "Value": "jobs",
- "Value #A": "jobs",
- "key": "author"
- }
- }
- },
- {
- "id": "sortBy",
- "options": {
- "fields": {},
- "sort": [
- {
- "desc": true,
- "field": "jobs"
- }
- ]
- }
- }
- ],
- "type": "table"
- },
{
"collapsed": false,
"gridPos": {
@@ -1114,6 +845,7 @@
"y": 45
},
"id": 18,
+ "options": {},
"targets": [
{
"datasource": {
@@ -1232,6 +964,7 @@
"y": 56
},
"id": 21,
+ "options": {},
"targets": [
{
"datasource": {
@@ -1269,8 +1002,268 @@
"content": "Sampled from the GitHub Actions REST API by `packages/observability/collectors/actions-queue.ts`, which emits one JSON line per observation on channel `boxel:actions-queue`.\n\nThe collector runs **outside** GitHub Actions on purpose — a scheduled workflow would queue behind the backlog it measures and go blind during the incident it exists for.\n\nDepth and grouped depth come from periodic snapshots, so each point is a sample rather than a continuous measure; gaps mean the collector was down or throttled (see *Collector health*).",
"mode": "markdown"
},
+ "targets": [],
"title": "About this dashboard",
"type": "text"
+ },
+ {
+ "collapsed": false,
+ "gridPos": {
+ "h": 1,
+ "w": 24,
+ "x": 0,
+ "y": 9
+ },
+ "id": 40,
+ "panels": [],
+ "title": "Right now",
+ "type": "row"
+ },
+ {
+ "datasource": {
+ "type": "loki",
+ "uid": "loki"
+ },
+ "description": "A point-in-time count, unlike the timeseries above — a branch whose jobs have finished disappears from this table instead of leaving a line behind.",
+ "fieldConfig": {
+ "defaults": {
+ "custom": {}
+ },
+ "overrides": [
+ {
+ "matcher": {
+ "id": "byName",
+ "options": "branch"
+ },
+ "properties": [
+ {
+ "id": "links",
+ "value": [
+ {
+ "targetBlank": true,
+ "title": "Actions runs for this branch",
+ "url": "https://github.com/cardstack/boxel/actions?query=branch%3A${__value.raw}"
+ }
+ ]
+ }
+ ]
+ }
+ ]
+ },
+ "gridPos": {
+ "h": 8,
+ "w": 8,
+ "x": 0,
+ "y": 10
+ },
+ "id": 41,
+ "interval": "5m",
+ "options": {},
+ "targets": [
+ {
+ "datasource": {
+ "type": "loki",
+ "uid": "loki"
+ },
+ "editorMode": "code",
+ "expr": "last_over_time({service=\"actions-collector\", env=\"$env\"} |= \"boxel:actions-queue\" | json | line_format \"{{ if .log }}{{ .log }}{{ else }}{{ __line__ }}{{ end }}\" | json | channel=\"boxel:actions-queue\" | event_type=\"group\" | dimension=\"branch\" | unwrap queued [5m]) by (key) > 0",
+ "queryType": "instant",
+ "refId": "A"
+ }
+ ],
+ "title": "Queued now, by branch",
+ "transformations": [
+ {
+ "id": "organize",
+ "options": {
+ "excludeByName": {
+ "Time": true
+ },
+ "renameByName": {
+ "Value": "jobs",
+ "Value #A": "jobs",
+ "key": "branch"
+ }
+ }
+ },
+ {
+ "id": "sortBy",
+ "options": {
+ "fields": {},
+ "sort": [
+ {
+ "desc": true,
+ "field": "jobs"
+ }
+ ]
+ }
+ }
+ ],
+ "type": "table"
+ },
+ {
+ "datasource": {
+ "type": "loki",
+ "uid": "loki"
+ },
+ "description": "Which branches are occupying the pool at this moment.",
+ "fieldConfig": {
+ "defaults": {
+ "custom": {}
+ },
+ "overrides": [
+ {
+ "matcher": {
+ "id": "byName",
+ "options": "branch"
+ },
+ "properties": [
+ {
+ "id": "links",
+ "value": [
+ {
+ "targetBlank": true,
+ "title": "Actions runs for this branch",
+ "url": "https://github.com/cardstack/boxel/actions?query=branch%3A${__value.raw}"
+ }
+ ]
+ }
+ ]
+ }
+ ]
+ },
+ "gridPos": {
+ "h": 8,
+ "w": 8,
+ "x": 8,
+ "y": 10
+ },
+ "id": 42,
+ "interval": "5m",
+ "options": {},
+ "targets": [
+ {
+ "datasource": {
+ "type": "loki",
+ "uid": "loki"
+ },
+ "editorMode": "code",
+ "expr": "last_over_time({service=\"actions-collector\", env=\"$env\"} |= \"boxel:actions-queue\" | json | line_format \"{{ if .log }}{{ .log }}{{ else }}{{ __line__ }}{{ end }}\" | json | channel=\"boxel:actions-queue\" | event_type=\"group\" | dimension=\"branch\" | unwrap running [5m]) by (key) > 0",
+ "queryType": "instant",
+ "refId": "A"
+ }
+ ],
+ "title": "Runners held now, by branch",
+ "transformations": [
+ {
+ "id": "organize",
+ "options": {
+ "excludeByName": {
+ "Time": true
+ },
+ "renameByName": {
+ "Value": "jobs",
+ "Value #A": "jobs",
+ "key": "branch"
+ }
+ }
+ },
+ {
+ "id": "sortBy",
+ "options": {
+ "fields": {},
+ "sort": [
+ {
+ "desc": true,
+ "field": "jobs"
+ }
+ ]
+ }
+ }
+ ],
+ "type": "table"
+ },
+ {
+ "datasource": {
+ "type": "loki",
+ "uid": "loki"
+ },
+ "description": "The same instant count grouped by whoever triggered the run.",
+ "fieldConfig": {
+ "defaults": {
+ "custom": {}
+ },
+ "overrides": [
+ {
+ "matcher": {
+ "id": "byName",
+ "options": "author"
+ },
+ "properties": [
+ {
+ "id": "links",
+ "value": [
+ {
+ "targetBlank": true,
+ "title": "GitHub profile",
+ "url": "https://github.com/${__value.raw}"
+ }
+ ]
+ }
+ ]
+ }
+ ]
+ },
+ "gridPos": {
+ "h": 8,
+ "w": 8,
+ "x": 16,
+ "y": 10
+ },
+ "id": 43,
+ "interval": "5m",
+ "options": {},
+ "targets": [
+ {
+ "datasource": {
+ "type": "loki",
+ "uid": "loki"
+ },
+ "editorMode": "code",
+ "expr": "last_over_time({service=\"actions-collector\", env=\"$env\"} |= \"boxel:actions-queue\" | json | line_format \"{{ if .log }}{{ .log }}{{ else }}{{ __line__ }}{{ end }}\" | json | channel=\"boxel:actions-queue\" | event_type=\"group\" | dimension=\"actor\" | unwrap queued [5m]) by (key) > 0",
+ "queryType": "instant",
+ "refId": "A"
+ }
+ ],
+ "title": "Queued now, by author",
+ "transformations": [
+ {
+ "id": "organize",
+ "options": {
+ "excludeByName": {
+ "Time": true
+ },
+ "renameByName": {
+ "Value": "jobs",
+ "Value #A": "jobs",
+ "key": "author"
+ }
+ }
+ },
+ {
+ "id": "sortBy",
+ "options": {
+ "fields": {},
+ "sort": [
+ {
+ "desc": true,
+ "field": "jobs"
+ }
+ ]
+ }
+ }
+ ],
+ "type": "table"
}
],
"refresh": "1m",
@@ -1285,7 +1278,7 @@
{
"hide": 2,
"name": "env",
- "query": "staging",
+ "query": "__ENV__",
"skipUrlSync": true,
"type": "constant"
}
(Run: https://github.com/cardstack/boxel/actions/runs/35616416867) |
Contributor
Grafana previewPreview deployed for 1 dashboard in the staging Grafana.
Dashboards: Preview is torn down automatically when this PR is closed or merged. (Run: https://github.com/cardstack/boxel/actions/runs/35616416911) |
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
2 participants
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
packages/base/scripts/check-declared-dependencies.mjs, wired aslint:declared-dependenciesalongside the existinglint:no-isused-option.#6220 (
date-fns) has merged, so this now targetsmaindirectly and the diff is the script plus itslint:wiring.The gap
packages/basecan import a package it does not declare, and nothing notices:import/no-extraneous-dependenciesis exactly this check, but it is not part ofplugin:import/recommended, nothing in the repo enables it, andeslint-plugin-importis not one of base's devDependencies.data-test-*, type-only imports — but dependency hygiene is a different axis and it says nothing about it.It works until something compiles base's source against base's own dependencies, since pnpm links only declared packages. Two have reached main that way, both found by bundling base into the host rather than by tooling:
ember-provide-consume-contextdate-fnsWhy a script and not the eslint rule
import/no-extraneous-dependenciesis the rule written for exactly this, and it structurally cannot catch this bug class under pnpm. I enabled it on base and measured rather than assuming:definitely-not-a-real-packagedate-fns, genuinely undeclaredOther rules fire on those same canary files, so the config was live. The reason is in the rule's source:
It silently skips anything it cannot resolve — and under pnpm an undeclared package is the unresolvable case.
require.resolve('date-fns')frompackages/basereturnsMODULE_NOT_FOUND, because pnpm links only declared dependencies and does not hoist. The rule's precondition and this bug's definition are mutually exclusive. What it does catch is a package that is installed but declared in the wrong manifest section, which is a different problem.import/no-unresolvedwould catch it, and is unusable for the opposite reason: enabled on base it reports 1,209 violations — 452@cardstack/*, 425 relative paths, 129@ember/*, 79@glimmer/*— nearly all legitimate, because base is card source and its imports resolve through the realm and the virtual network rather than node_modules. That is why host disables it with the comment that it "doesn't work well with the monorepo".So: the rule designed for this cannot see it, and the rule that can see it cannot tell a legitimate card-source import from a broken one. Hence a check that knows the four idioms:
@ember/*,@glimmer/*shipped insideember-source@ember/object@cardstack/boxel-host/commands/*https://esm.run/currency-code-symbol-map@cardstack/base/brand-guide@cardstack/boxel-hostcannot be declared in any case — host already depends on base, so it would be a cycle.Proof it works
Removing either declaration makes it fail and name the importing files:
Import extraction is tested against each shape it has to handle. It finds single-line, multi-line,
import type, default-plus-named, side-effect,export … from,export *, namespace, dynamicimport()and URL specifiers.Comments and template-literal bodies are blanked before scanning. That is done as a scan rather than a regex because the two interact:
//inside a string starts no comment, and base imports from URLs (https://esm.run/…), so a naive comment strip would truncate a real specifier. Angle brackets are excluded from the gap beforefrom, which covers Glimmer templates — those are not JS strings, so they survive the blanking.Four false positives raised in review now extract nothing, verified against the script's own regexes rather than a copy of them:
export class A {}then// borrowed from 'some-pkg'some-pkg<template>containingAdapted from "the style guide"the style guide`see import('lazy-pkg') for details`lazy-pkg/* see import('block-pkg') */block-pkgKnown limits
Regex over blanked source, not an AST, so an import assembled at runtime (
import(someVariable)) is invisible. That is a deliberate trade for a check with no new dependency, matching thecheck-no-isused-option.mjsprecedent. It catches the static case, which is both bugs seen so far.🤖 Generated with Claude Code