docs(health): document field-name drift vs ManagedAccount (AUDIT-M08)#408
docs(health): document field-name drift vs ManagedAccount (AUDIT-M08)#408
Conversation
…/ D-04) The master audit (dim-D-routing.md D-04, AUDIT-M08) flagged that lib/health.ts uses field names that do not exactly match ManagedAccount in lib/accounts.ts — specifically: getAccountHealth input ManagedAccount field ---------------------- -------------------- rateLimitedUntil <computed from rateLimitResetTimes per family> cooldownUntil coolingDownUntil lastUsedAt lastUsed health <computed from getHealthTracker().getScore()> Oracle's recommended fix was either "reads from tracker state directly OR module deleted". Neither is appropriate here: - Deleting would break external consumers (the module is exported via lib/index.ts and documented as public API). - Changing the signature would also break public API. - Adding a parallel `getPluginHealthFromManager(manager)` function is plausible but has no internal consumer today and would duplicate the shape-transformation logic. The pragmatic fix is to DOCUMENT the drift explicitly in the module so the next contributor who reads this code understands the design choice: this is a PURE SHAPE-TRANSFORMATION UTILITY that deliberately takes a flattened input so callers can build it from any source. The comment also calls out the exact field-name mappings and tells future implementers what to do if they want a direct-from-manager variant (add a new function, do NOT change the existing signature). This closes the audit's observability gap (the drift is now discoverable from the code) without a breaking change. When an internal consumer eventually needs a direct-from-manager bridge, the guidance in the comment tells them exactly how to add it safely. Closes AUDIT-M08 / D-04 (health module field-name drift vs live AccountManager state) at the documentation level. Evidence: 225/225 test files, 3418/3418 tests pass (docs-only change). typecheck + lint exit 0. No new dependencies, no API changes.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughSeverity: Minor (Documentation-only) This PR adds a module-level comment to Summary Adds a multi-line in-file comment explaining:
Verification Full test suite: 225/225 files, 3,418/3418 tests passing; npm typecheck and lint both exit cleanly. Walkthroughmodule-level docstring added to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/health.ts`:
- Around line 4-30: The docstring documents field-name mappings between
ManagedAccount and the public getAccountHealth input but offers no compile-time
guarantee; add a type-level regression test that imports type ManagedAccount and
the getAccountHealth function and constructs the getAccountHealth input directly
from a ManagedAccount instance (e.g., a small function like
testFieldMapping(managed: ManagedAccount) { getAccountHealth([ { index:
managed.index, email: managed.email, accountId: managed.accountId, refreshToken:
managed.refreshToken, health: managed.getHealthTracker().getScore(),
rateLimitedUntil: /* map from managed.rateLimitResetTimes */, cooldownUntil:
managed.coolingDownUntil, cooldownReason: managed.cooldownReason, lastUsedAt:
managed.lastUsed } ]) } ) so any rename in ManagedAccount will break compilation
and surface the drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 575a893f-8901-44e4-8311-ffe00c886ca2
📒 Files selected for processing (1)
lib/health.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/health.ts
🔇 Additional comments (1)
lib/health.ts (1)
4-30: the field mappings in the docstring are accurate and properly tested.verified against the actual code:
lastUsedAt→lastUsedconfirmed (lib/accounts.ts:245)rateLimitedUntil←rateLimitResetTimesconfirmed (lib/accounts.ts:253)cooldownUntil→coolingDownUntilconfirmed (lib/accounts.ts:254)healthinput field used throughout (test/health.test.ts validates it across multiple scenarios)export via lib/index.ts:25 confirmed. no token/email leaks in the docstring. tests in test/health.test.ts cover all field mappings with green results.
| // AUDIT-M08 / D-04 docstring (master repository audit, Oracle-confirmed): | ||
| // -------------------------------------------------------------------------- | ||
| // This module is a PURE SHAPE-TRANSFORMATION UTILITY. Callers pass an | ||
| // already-flattened accounts array with the fields defined in the public | ||
| // `getAccountHealth` parameter shape below. It is deliberately decoupled | ||
| // from the live AccountManager: that keeps the function easy to test, and | ||
| // lets external consumers (dashboards, diagnostics scripts, JSON exporters) | ||
| // build the input from any source without taking a hard dependency on the | ||
| // manager singleton. | ||
| // | ||
| // HOWEVER, the parameter shape uses field names that do NOT exactly match | ||
| // ManagedAccount in lib/accounts.ts: | ||
| // | ||
| // getAccountHealth input ManagedAccount field | ||
| // ---------------------- -------------------- | ||
| // rateLimitedUntil <computed from rateLimitResetTimes per family> | ||
| // cooldownUntil coolingDownUntil | ||
| // lastUsedAt lastUsed | ||
| // health <computed from getHealthTracker().getScore()> | ||
| // | ||
| // Callers are responsible for flattening ManagedAccount → the input shape | ||
| // via a small adapter. If a future refactor wants a single call site that | ||
| // reads straight from AccountManager, add a `getPluginHealthFromManager( | ||
| // manager: AccountManager): PluginHealth` function here and implement the | ||
| // flattening inline. Do NOT change the signature of the existing | ||
| // `getAccountHealth` function without a deprecation cycle — it is exported | ||
| // via lib/index.ts and is considered public API. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
docstring improves observability but doesn't prevent future drift.
the comment correctly documents the current field-name mappings (verified lastUsedAt → lastUsed at lib/health.ts:80), but the audit gap will reopen if someone renames fields in ManagedAccount without updating this comment.
consider adding a type-level test or assertion that forces a compile error if the mappings diverge. for example, in your test suite, add a static check that constructs the input shape from a ManagedAccount instance to catch renames.
🔒 example type-level test to prevent drift
add to your test file:
// type-level regression test for AUDIT-M08 field mapping
import type { ManagedAccount } from '../lib/accounts.js';
import { getAccountHealth } from '../lib/health.js';
// this will fail to compile if ManagedAccount renames fields
// without updating the adapter
function testFieldMapping(managed: ManagedAccount) {
getAccountHealth([{
index: managed.index,
email: managed.email,
accountId: managed.accountId,
refreshToken: managed.refreshToken,
health: managed.getHealthTracker().getScore(), // computed
rateLimitedUntil: managed.rateLimitResetTimes?.['family']?.resetAt, // computed
cooldownUntil: managed.coolingDownUntil, // renamed field
cooldownReason: managed.cooldownReason,
lastUsedAt: managed.lastUsed, // renamed field
}]);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/health.ts` around lines 4 - 30, The docstring documents field-name
mappings between ManagedAccount and the public getAccountHealth input but offers
no compile-time guarantee; add a type-level regression test that imports type
ManagedAccount and the getAccountHealth function and constructs the
getAccountHealth input directly from a ManagedAccount instance (e.g., a small
function like testFieldMapping(managed: ManagedAccount) { getAccountHealth([ {
index: managed.index, email: managed.email, accountId: managed.accountId,
refreshToken: managed.refreshToken, health:
managed.getHealthTracker().getScore(), rateLimitedUntil: /* map from
managed.rateLimitResetTimes */, cooldownUntil: managed.coolingDownUntil,
cooldownReason: managed.cooldownReason, lastUsedAt: managed.lastUsed } ]) } ) so
any rename in ManagedAccount will break compilation and surface the drift.
1 participant
Summary
Documents the field-name drift between
lib/health.tsandlib/accounts.ts'sManagedAccountin an explicit in-file comment. This closes the audit's observability gap (AUDIT-M08 / D-04) without a breaking API change.Problem
Audit (
docs/audits/MASTER_AUDIT.md§6 MEDIUMAUDIT-M08/dim-D-routing.mdD-04) identified thatgetAccountHealth()inlib/health.tsuses parameter field names that do not exactly matchManagedAccount:rateLimitedUntilrateLimitResetTimesper familycooldownUntilcoolingDownUntillastUsedAtlastUsedhealthgetHealthTracker().getScore()The drift is not a bug — the function is a pure shape-transformation utility and callers are responsible for the flattening. But the mismatch was silent and not discoverable from the code.
Why not Oracle's recommended fix?
Oracle's options were "reads from tracker state directly OR module deleted":
lib/index.tsand documented as public API for dashboards / diagnostics / JSON exporters.getPluginHealthFromManager(manager)— plausible, but no internal consumer today; adding it would duplicate the shape-transformation logic without a live caller.The pragmatic fix is documentation: make the drift discoverable from the code, and give future implementers explicit guidance on how to add a direct-from-manager bridge safely when an internal consumer eventually needs one.
Change
lib/health.tsgets a multi-line comment block before the exported interfaces that:getAccountHealthinput andManagedAccount.getPluginHealthFromManager(manager), do NOT change the existing signature.No code changes. No behavioral change. No new dependencies.
Verification
npm run typecheckexit 0npm run lintexit 0Audit reference
docs/audits/MASTER_AUDIT.md§6 MEDIUMAUDIT-M08docs/audits/evidence/dim-D-routing.mdD-04docs/audits/evidence/oracle-verdicts.md§1.2Scope guarantees
Follow-up
Phase 1 remaining architectural work:
JSON.parseboundaries (R3)why-selected+verify --pathsfeatures (F1, F2)Tracked in
.sisyphus/plans/phase1-implementation.md.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
documentation-only change adds a module-level comment block to
lib/health.tsthat codifies the field-name drift between thegetAccountHealthinput shape andManagedAccount(AUDIT-M08 / D-04). the four-row mapping table is accurate againstlib/accounts.ts, the export claim matcheslib/index.ts, and the guidance for a futuregetPluginHealthFromManagerbridge is sound. no code, no behavior, no api changes.Confidence Score: 5/5
safe to merge — docs-only, zero behavioral or api change, all field mappings verified correct
no code changes; the four documented field-name mappings are accurate against lib/accounts.ts; the public-api export claim is confirmed via lib/index.ts; existing test suite unaffected; all p2 or lower
no files require special attention
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD MA["ManagedAccount\n(lib/accounts.ts)"] ADAPTER["caller adapter\n(flatten to input shape)"] GH["getAccountHealth(accounts[])"] AH["AccountHealth[]"] MA -->|"lastUsed → lastUsedAt"| ADAPTER MA -->|"coolingDownUntil → cooldownUntil"| ADAPTER MA -->|"rateLimitResetTimes (per-family) → rateLimitedUntil"| ADAPTER MA -->|"getHealthTracker().getScore() → health"| ADAPTER ADAPTER --> GH GH --> AHReviews (1): Last reviewed commit: "docs(health): document field-name drift ..." | Re-trigger Greptile