Restore SmrtObject base fields and clean workspace test warnings

[willgriffin]

Summary

• restore SMRT system fields for getAllFields() without reintroducing SmrtObject into inheritance chains
• add regression coverage for issue Bug: getAllFields() omits SmrtObject base fields like slug/id for descendants #1083 and update related inheritance/manifest expectations
• clean workspace test tooling warnings by removing the repo-level NPM_TOKEN interpolation, replacing npx vitest wrappers, and making @happyvertical/smrt-users rebuild @happyvertical/smrt-profiles after a clean run

Testing

• pnpm run clean
• pnpm test

Closes #1083.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a51b7ca1e9

ℹ️ 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".

[Copilot]

Pull request overview

This PR restores canonical SmrtObject system fields (id, slug, context, created_at, updated_at) in ObjectRegistry.getAllFields() without reintroducing SmrtObject into inheritance chains, adds regression tests for the reported metadata mismatch (issue #1083), and reduces workspace testing/tooling warnings by removing npx wrappers and repo-level GitHub Packages token interpolation.

Changes:

• Add a shared system-fields helper and prepend SMRT system fields to getAllFields() results.
• Update schema generation and tests to align with system-field visibility and new inheritance/manifest expectations.
• Clean up workspace scripts/config: switch npx vitest → vitest, update manifest generator scripts to avoid npx, and remove ${NPM_TOKEN} interpolation from .npmrc.

Reviewed changes

Copilot reviewed 44 out of 45 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pnpm-lock.yaml	Lockfile updates reflecting dependency graph/script/config changes.
.npmrc	Removes repo-level auth token interpolation; keeps registry mapping.
packages/ads/package.json	Replace npx vitest with vitest in scripts.
packages/affiliates/package.json	Replace npx vitest with vitest in scripts.
packages/agents/package.json	Replace npx vitest with vitest in scripts.
packages/analytics/package.json	Replace npx vitest with vitest in scripts (incl. DB project variants).
packages/assets/package.json	Replace npx vitest with vitest in scripts.
packages/chat/package.json	Replace npx vitest with vitest in scripts.
packages/cli/package.json	Replace npx vitest with vitest in scripts.
packages/commerce/package.json	Replace npx vitest with vitest in scripts.
packages/content/package.json	Replace npx vitest with vitest in scripts (after svelte-kit sync).
packages/core/package.json	Replace npx vitest with vitest in test scripts.
packages/core/scripts/generate-manifest.js	Run TS generator via node --import tsx and use unlinkSync for cleanup.
packages/core/scripts/generate-test-manifest.js	Run TS generator via node --import tsx (avoids npx tsx).
packages/core/src/system-fields.ts	New canonical SMRT system-field definitions + helpers.
packages/core/src/utils.ts	Removes duplicated base-field injection from fieldsFromClass() (now relies on registry).
packages/core/src/registry.ts	Prepends system fields to ObjectRegistry.getAllFields() output.
packages/core/src/registry/inheritance-resolver.ts	Prepends system fields to module-level getAllFields() output.
packages/core/src/schema/generator.ts	Ensures timestamp columns are emitted even when system fields appear in registry field maps.
packages/core/src/tests/issue-1083-smrtobject-base-fields.test.ts	New regression test ensuring getAllFields() includes system fields for descendants.
packages/core/src/tests/issue-265-external-package-manifest.test.ts	Updates expectations: system fields remain visible even when local manifest data is absent.
packages/core/src/tests/inheritance.test.ts	Updates inheritance expectations to include system fields while keeping internal props excluded.
packages/events/package.json	Replace npx vitest with vitest in scripts.
packages/facts/package.json	Replace npx vitest with vitest in scripts.
packages/gnode/package.json	Replace npx vitest with vitest in scripts.
packages/images/package.json	Replace npx vitest with vitest in scripts.
packages/jobs/package.json	Replace npx vitest with vitest in scripts.
packages/ledgers/package.json	Replace npx vitest with vitest in scripts.
packages/messages/package.json	Replace npx vitest with vitest in scripts.
packages/places/package.json	Replace npx vitest with vitest in scripts.
packages/profiles/package.json	Replace npx vitest with vitest in scripts.
packages/properties/package.json	Replace npx vitest with vitest in scripts.
packages/secrets/package.json	Replace npx vitest with vitest in scripts.
packages/sites/package.json	Replace npx vitest with vitest in scripts.
packages/smrt-dev-mcp/package.json	Replace npx vitest with vitest in scripts.
packages/smrt-playground/package.json	Replace npx vitest with vitest in scripts.
packages/smrt-svelte/package.json	Replace npx vitest with vitest in scripts.
packages/social/package.json	Replace npx vitest with vitest in scripts.
packages/tags/package.json	Replace npx vitest with vitest in scripts.
packages/tenancy/package.json	Replace npx vitest with vitest in scripts.
packages/types/package.json	Replace npx vitest with vitest in scripts.
packages/users/package.json	Replace npx vitest with vitest; add @happyvertical/smrt-profiles to dev deps for workspace/test rebuild behavior.
packages/video/package.json	Replace npx vitest with vitest in scripts.
packages/voice/package.json	Replace npx vitest with vitest in scripts.

Files not reviewed (1)

• pnpm-lock.yaml: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Now that getAllFields() prepends SmrtObject system fields, generateSchemaFromRegistry() will also see those implicit id/slug/context entries. When hasCustomPK is true, the current logic stops auto-adding the standard SMRT columns, but the injected system fields can still fall through the main loop and generate nullable id/slug/context columns (since their _meta has no required). Consider marking injected system fields (e.g. _meta.__smrtSystemField = true) so schema generation can either skip them when hasCustomPK is set or apply the same non-null/default semantics intentionally, while still allowing explicit user-declared fields to win.

[willgriffin]

Addressed in 2630eae3. The injected base fields now carry _meta.__smrtSystemField, and the CTI schema generator skips only those synthetic id/slug/context fields when hasCustomPK is true. created_at / updated_at still get their normal timestamp handling, and explicit user-declared fields with the same names are preserved.

[willgriffin]

packages/cli/src/commands/utilities.ts — hardcoded vitest.mjs path may break with hoisted deps

The switch from npx vitest to direct Node execution is a nice improvement, but hardcoding process.cwd() + "/node_modules/vitest/vitest.mjs" can be fragile in monorepo setups. If pnpm hoists vitest to the workspace root (or a consumer runs smrt test in an app folder where vitest lives elsewhere), this access() will throw and the command silently falls through to ❌ Could not find or run vitest.

Consider using Node's module resolution instead:

import { createRequire } from "node:module";
// ...
const require = createRequire(resolve(process.cwd(), "package.json"));
const vitestEntrypoint = require.resolve("vitest/vitest.mjs");
await access(vitestEntrypoint);

This lets Node figure out where vitest actually lives, regardless of hoisting strategy.

---

Everything else looks solid — the system-fields.ts extraction is clean, the schema generator custom-PK logic is correct, and the npx → bare vitest / pnpm exec sweep across package.json scripts is a welcome cleanup. 👍

[willgriffin]

PR Review: Restore SmrtObject base fields and clean workspace test warnings

Overall this is a well-structured PR that cleanly separates the system-fields concern into its own module. The tooling cleanup is comprehensive. A few observations below:

---

1. prependSmrtSystemFields called at two independent getAllFields exit points

Both ObjectRegistry.getAllFields() in registry.ts and the standalone getAllFields() in inheritance-resolver.ts now call prependSmrtSystemFields() before returning. Since these are independent code paths (the static method does not delegate to the standalone function), there is no double-prepend problem.

However, both include an early-return via inheritedFields cache (e.g. registry.ts:1780) that bypasses prependSmrtSystemFields. If that cache is populated by a code path that also goes through prependSmrtSystemFields, the system fields are already present in the cache and the early-return is fine. If the cache is ever populated without going through one of these two returns (e.g., directly by the vitest plugin or manifest loading), the early-return path will omit system fields. Worth confirming this is covered by the regression test for the cached path.

---

2. Schema generator: duplicated timestamp handling blocks

The continue statements added for created_at/updated_at in the schema generator are correct — previously these fields would fall through to the generic column builder after setting the dedup flag, potentially creating duplicate or mistyped columns. However, the identical 7-line column definition block appears four times across the two SchemaGenerator methods (two for created_at, two for updated_at). Consider extracting to a helper like buildTimestampColumn(name, description) to reduce drift risk.

---

3. Custom PK guard uses sentinel to filter injected system fields — nice

The schema generator condition:

if (isDefaultSmrtColumn && (!hasCustomPK || field._meta?.__smrtSystemField === true)) {
  continue;
}

correctly distinguishes between injected system fields (which should be skipped when a custom PK is present) and explicitly declared id/slug/context fields (which should survive). The new CustomPrimaryKeyRecord test validates this well.

---

4. fieldsFromClass strips __smrtSystemField — good hygiene

The utils.ts change removes the old BASE_FIELDS inline constant (now redundant with system-fields.ts) and strips the __smrtSystemField sentinel from _meta before returning. This prevents the internal marker from leaking into downstream consumers. Clean.

---

5. Tooling cleanup is thorough

• execSync → execFileSync with process.execPath avoids shell injection and npx resolution overhead
• npx vitest → vitest / pnpm exec vitest across all packages is consistent
• .npmrc no longer interpolates NPM_TOKEN (moved to CI setup action) — eliminates the warning for local dev
• smrt-profiles moved from dependencies to devDependencies in smrt-users — correct since it's only needed for test manifest generation
• pnpm 9 → 10 bump is a lockfile-only change here

---

6. Minor: isInjectedSmrtSystemField is exported but unused

system-fields.ts exports isInjectedSmrtSystemField() which is a clean predicate, but it's not used anywhere in this PR. If it's intended for downstream consumers, that's fine — just confirming it's intentional rather than a leftover from an earlier approach.

---

Overall LGTM — the system-fields module is well-designed, the regression tests are solid, and the tooling cleanup removes real friction. 👍

[Copilot]

Pull request overview

Copilot reviewed 48 out of 49 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• pnpm-lock.yaml: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.