Skip to content

Commit a04f01c

Browse files
hesreallyhimclaude
hesreallyhim
and
claude
committed
fix: address code review findings (batch 2)
- Fix path construction in spawnPoller: use path.join() instead of manual string concatenation with separator logic - Improve state validation: validate individual BucketState entries inside isValidState, remove misleading TODO comment - Make parseRateLimitResponse resilient to partial failures: skip invalid resources with continue instead of returning null Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent c05b819 commit a04f01c

7 files changed

Lines changed: 147 additions & 21 deletions

File tree

‎dist/poller/index.js‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ function parseRateLimitResponse(raw) {
158158
const resources = {};
159159
for (const [key, value] of Object.entries(raw['resources'])) {
160160
if (!isValidSample(value)) {
161-
return null;
161+
continue; // Skip invalid resources instead of failing the entire response
162162
}
163163
resources[key] = value;
164164
}
@@ -489,8 +489,6 @@ function readState() {
489489
try {
490490
const content = external_fs_namespaceObject.readFileSync(statePath, 'utf-8');
491491
const parsed = JSON.parse(content);
492-
// TODO: Validate parsed state has correct shape
493-
// For now, trust the structure
494492
if (!isValidState(parsed)) {
495493
return {
496494
success: false,
@@ -579,6 +577,12 @@ function isValidState(obj) {
579577
if (typeof obj['poll_failures'] !== 'number') {
580578
return false;
581579
}
580+
// Validate each bucket entry
581+
for (const value of Object.values(obj['buckets'])) {
582+
if (!isValidBucketState(value)) {
583+
return false;
584+
}
585+
}
582586
// Optional fields: must be string | null
583587
if (!isStringOrNull(obj['stopped_at_ts'])) {
584588
return false;
@@ -591,6 +595,34 @@ function isValidState(obj) {
591595
}
592596
return true;
593597
}
598+
/**
599+
* Validates that a value has the BucketState shape.
600+
*/
601+
function isValidBucketState(value) {
602+
if (!isARealObject(value)) {
603+
return false;
604+
}
605+
const numericFields = [
606+
'last_reset',
607+
'last_used',
608+
'total_used',
609+
'windows_crossed',
610+
'anomalies',
611+
'limit',
612+
'remaining',
613+
'first_used',
614+
'first_remaining',
615+
];
616+
for (const field of numericFields) {
617+
if (typeof value[field] !== 'number') {
618+
return false;
619+
}
620+
}
621+
if (typeof value['last_seen_ts'] !== 'string') {
622+
return false;
623+
}
624+
return true;
625+
}
594626
// -----------------------------------------------------------------------------
595627
// PID file management
596628
// -----------------------------------------------------------------------------
@@ -770,8 +802,7 @@ function spawnPoller(token) {
770802
const baseDir = actionPath
771803
? path.resolve(actionPath, 'dist')
772804
: path.dirname(process.argv[1] ?? '');
773-
const separator = baseDir.endsWith(path.sep) ? '' : path.sep;
774-
const pollerEntry = `${baseDir}${separator}poller${path.sep}index.js`;
805+
const pollerEntry = path.join(baseDir, 'poller', 'index.js');
775806
const child = spawn(process.execPath, [pollerEntry], {
776807
detached: true,
777808
stdio: 'ignore',

‎dist/post.js‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30974,8 +30974,6 @@ function state_readState() {
3097430974
try {
3097530975
const content = external_fs_namespaceObject.readFileSync(statePath, 'utf-8');
3097630976
const parsed = JSON.parse(content);
30977-
// TODO: Validate parsed state has correct shape
30978-
// For now, trust the structure
3097930977
if (!isValidState(parsed)) {
3098030978
return {
3098130979
success: false,
@@ -31064,6 +31062,12 @@ function isValidState(obj) {
3106431062
if (typeof obj['poll_failures'] !== 'number') {
3106531063
return false;
3106631064
}
31065+
// Validate each bucket entry
31066+
for (const value of Object.values(obj['buckets'])) {
31067+
if (!isValidBucketState(value)) {
31068+
return false;
31069+
}
31070+
}
3106731071
// Optional fields: must be string | null
3106831072
if (!isStringOrNull(obj['stopped_at_ts'])) {
3106931073
return false;
@@ -31076,6 +31080,34 @@ function isValidState(obj) {
3107631080
}
3107731081
return true;
3107831082
}
31083+
/**
31084+
* Validates that a value has the BucketState shape.
31085+
*/
31086+
function isValidBucketState(value) {
31087+
if (!isARealObject(value)) {
31088+
return false;
31089+
}
31090+
const numericFields = [
31091+
'last_reset',
31092+
'last_used',
31093+
'total_used',
31094+
'windows_crossed',
31095+
'anomalies',
31096+
'limit',
31097+
'remaining',
31098+
'first_used',
31099+
'first_remaining',
31100+
];
31101+
for (const field of numericFields) {
31102+
if (typeof value[field] !== 'number') {
31103+
return false;
31104+
}
31105+
}
31106+
if (typeof value['last_seen_ts'] !== 'string') {
31107+
return false;
31108+
}
31109+
return true;
31110+
}
3107931111
// -----------------------------------------------------------------------------
3108031112
// PID file management
3108131113
// -----------------------------------------------------------------------------
@@ -31255,8 +31287,7 @@ function spawnPoller(token) {
3125531287
const baseDir = actionPath
3125631288
? path.resolve(actionPath, 'dist')
3125731289
: path.dirname(process.argv[1] ?? '');
31258-
const separator = baseDir.endsWith(path.sep) ? '' : path.sep;
31259-
const pollerEntry = `${baseDir}${separator}poller${path.sep}index.js`;
31290+
const pollerEntry = path.join(baseDir, 'poller', 'index.js');
3126031291
const child = spawn(process.execPath, [pollerEntry], {
3126131292
detached: true,
3126231293
stdio: 'ignore',
@@ -31883,7 +31914,7 @@ function parseRateLimitResponse(raw) {
3188331914
const resources = {};
3188431915
for (const [key, value] of Object.entries(raw['resources'])) {
3188531916
if (!isValidSample(value)) {
31886-
return null;
31917+
continue; // Skip invalid resources instead of failing the entire response
3188731918
}
3188831919
resources[key] = value;
3188931920
}

‎dist/pre.js‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30974,8 +30974,6 @@ function state_readState() {
3097430974
try {
3097530975
const content = external_fs_namespaceObject.readFileSync(statePath, 'utf-8');
3097630976
const parsed = JSON.parse(content);
30977-
// TODO: Validate parsed state has correct shape
30978-
// For now, trust the structure
3097930977
if (!isValidState(parsed)) {
3098030978
return {
3098130979
success: false,
@@ -31064,6 +31062,12 @@ function isValidState(obj) {
3106431062
if (typeof obj['poll_failures'] !== 'number') {
3106531063
return false;
3106631064
}
31065+
// Validate each bucket entry
31066+
for (const value of Object.values(obj['buckets'])) {
31067+
if (!isValidBucketState(value)) {
31068+
return false;
31069+
}
31070+
}
3106731071
// Optional fields: must be string | null
3106831072
if (!isStringOrNull(obj['stopped_at_ts'])) {
3106931073
return false;
@@ -31076,6 +31080,34 @@ function isValidState(obj) {
3107631080
}
3107731081
return true;
3107831082
}
31083+
/**
31084+
* Validates that a value has the BucketState shape.
31085+
*/
31086+
function isValidBucketState(value) {
31087+
if (!isARealObject(value)) {
31088+
return false;
31089+
}
31090+
const numericFields = [
31091+
'last_reset',
31092+
'last_used',
31093+
'total_used',
31094+
'windows_crossed',
31095+
'anomalies',
31096+
'limit',
31097+
'remaining',
31098+
'first_used',
31099+
'first_remaining',
31100+
];
31101+
for (const field of numericFields) {
31102+
if (typeof value[field] !== 'number') {
31103+
return false;
31104+
}
31105+
}
31106+
if (typeof value['last_seen_ts'] !== 'string') {
31107+
return false;
31108+
}
31109+
return true;
31110+
}
3107931111
// -----------------------------------------------------------------------------
3108031112
// PID file management
3108131113
// -----------------------------------------------------------------------------
@@ -31255,8 +31287,7 @@ function spawnPoller(token) {
3125531287
const baseDir = actionPath
3125631288
? external_path_namespaceObject.resolve(actionPath, 'dist')
3125731289
: external_path_namespaceObject.dirname(process.argv[1] ?? '');
31258-
const separator = baseDir.endsWith(external_path_namespaceObject.sep) ? '' : external_path_namespaceObject.sep;
31259-
const pollerEntry = `${baseDir}${separator}poller${external_path_namespaceObject.sep}index.js`;
31290+
const pollerEntry = external_path_namespaceObject.join(baseDir, 'poller', 'index.js');
3126031291
const child = (0,external_child_process_namespaceObject.spawn)(process.execPath, [pollerEntry], {
3126131292
detached: true,
3126231293
stdio: 'ignore',
@@ -31883,7 +31914,7 @@ function parseRateLimitResponse(raw) {
3188331914
const resources = {};
3188431915
for (const [key, value] of Object.entries(raw['resources'])) {
3188531916
if (!isValidSample(value)) {
31886-
return null;
31917+
continue; // Skip invalid resources instead of failing the entire response
3188731918
}
3188831919
resources[key] = value;
3188931920
}

‎src/github.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export function parseRateLimitResponse(raw: unknown): RateLimitResponse | null {
140140

141141
for (const [key, value] of Object.entries(raw['resources'])) {
142142
if (!isValidSample(value)) {
143-
return null;
143+
continue; // Skip invalid resources instead of failing the entire response
144144
}
145145
resources[key] = value;
146146
}

‎src/poller.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,7 @@ export function spawnPoller(token: string): SpawnOutcome {
5757
const baseDir = actionPath
5858
? path.resolve(actionPath, 'dist')
5959
: path.dirname(process.argv[1] ?? '');
60-
const separator = baseDir.endsWith(path.sep) ? '' : path.sep;
61-
const pollerEntry = `${baseDir}${separator}poller${path.sep}index.js`;
60+
const pollerEntry = path.join(baseDir, 'poller', 'index.js');
6261

6362
const child: ChildProcess = spawn(process.execPath, [pollerEntry], {
6463
detached: true,

‎src/state.ts‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@ export function readState(): ReadStateOutcome {
4444
const content = fs.readFileSync(statePath, 'utf-8');
4545
const parsed = JSON.parse(content) as unknown;
4646

47-
// TODO: Validate parsed state has correct shape
48-
// For now, trust the structure
4947
if (!isValidState(parsed)) {
5048
return {
5149
success: false,
@@ -156,6 +154,13 @@ export function isValidState(obj: unknown): obj is ReducerState {
156154
return false;
157155
}
158156

157+
// Validate each bucket entry
158+
for (const value of Object.values(obj['buckets'])) {
159+
if (!isValidBucketState(value)) {
160+
return false;
161+
}
162+
}
163+
159164
// Optional fields: must be string | null
160165
if (!isStringOrNull(obj['stopped_at_ts'])) {
161166
return false;
@@ -170,6 +175,35 @@ export function isValidState(obj: unknown): obj is ReducerState {
170175
return true;
171176
}
172177

178+
/**
179+
* Validates that a value has the BucketState shape.
180+
*/
181+
function isValidBucketState(value: unknown): boolean {
182+
if (!isARealObject(value)) {
183+
return false;
184+
}
185+
const numericFields = [
186+
'last_reset',
187+
'last_used',
188+
'total_used',
189+
'windows_crossed',
190+
'anomalies',
191+
'limit',
192+
'remaining',
193+
'first_used',
194+
'first_remaining',
195+
];
196+
for (const field of numericFields) {
197+
if (typeof value[field] !== 'number') {
198+
return false;
199+
}
200+
}
201+
if (typeof value['last_seen_ts'] !== 'string') {
202+
return false;
203+
}
204+
return true;
205+
}
206+
173207
// -----------------------------------------------------------------------------
174208
// PID file management
175209
// -----------------------------------------------------------------------------

‎test/github.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ describe('parseRateLimitResponse', () => {
119119
expect(parseRateLimitResponse({ rate: {} })).toBeNull();
120120
});
121121

122-
it('returns null for invalid resource sample', () => {
122+
it('returns null when all resources are invalid and rate is invalid', () => {
123123
expect(
124124
parseRateLimitResponse({
125125
resources: { core: { limit: 'invalid' } },

0 commit comments

Comments
 (0)