Search code, repositories, users, issues, pull requests...

Line 180 in 3760d50

assertString(entry.id, `${prefix}.id`, configPath);

Allow service repos without explicit id

validateServiceConfig now hard-requires repos[i].id, but the service runtime can resolve repos by path and already falls back to path-based identity (resolveRepoEntry in tools/service/repos.js). This turns valid path-only repo entries into hard startup failures for indexer-service, which is a behavior regression for existing local-only configs.

Line 182 in 3760d50

assertString(entry.url, `${prefix}.url`, configPath);

Stop requiring repo URL for local service repos

Requiring repos[i].url at config-parse time breaks legitimate local workflows where repos are already present on disk (or syncPolicy: none is used), because ensureRepo only needs a URL when it must clone a missing repo. With this validation, those previously valid configs are rejected before the service starts.

Line 183 in 3760d50

assertString(entry.branch, `${prefix}.branch`, configPath);

Permit omitted branch in service repo entries

The new validation makes repos[i].branch mandatory even though the runtime intentionally has a default branch fallback (entry.branch || 'main' in ensureRepo). This rejects valid configs that relied on the existing defaulting behavior and causes avoidable bootstrap failures.

Line 184 in 3760d50

    
           assertPolicy(entry.syncPolicy, `${prefix}.syncPolicy`, configPath, SERVICE_SYNC_POLICIES);

Normalize repo syncPolicy after case-insensitive validation

assertPolicy accepts case-insensitive values, so syncPolicy: "FETCH" now passes validation, but the value is never normalized before use; ensureRepo compares exact lowercase values and will treat non-normalized strings as pull. This means accepted config values can silently execute the wrong sync action.

Line 198 in 3760d50

    
           assertPolicy(config.sync.policy, 'sync.policy', configPath, SERVICE_SYNC_POLICIES);

Normalize sync.policy after case-insensitive validation

sync.policy is validated case-insensitively but merged back without normalization, so values like "FETCH" are accepted yet later interpreted via strict lowercase checks and can degrade to pull behavior. This creates a mismatch between accepted configuration and effective runtime policy.

Lines 108 to 111 in 3760d50

    
           const assertNonNegativeInteger = (value, label, configPath, { min = 0 } = {}) => { 
        
             if (value == null) return; 
        
             const parsed = Number(value); 
        
             if (!Number.isFinite(parsed) || Math.floor(parsed) !== parsed || parsed < min) {

Coerce validated numeric fields before storing config

assertNonNegativeInteger validates by coercing with Number(value), so numeric strings are accepted, but loadServiceConfig then preserves raw values and downstream code often checks Number.isFinite on the uncoerced field. In practice, configs that pass validation (e.g. "4") can be silently ignored at runtime.

Lines 735 to 737 in 3760d50

    
           'includeRiskPartialFlows', 
        
           'strictRisk', 
        
           'rule',

Whitelist strictEvidence for context-pack command

The CLI wrapper validates pairofcleats context-pack flags against this allowlist, but strictEvidence is missing even though runContextPackCli supports it. As a result, users cannot enable strict evidence mode through the top-level CLI and get an unknown-flag error before reaching the actual command.

Lines 760 to 764 in 3760d50

    
             'maxPaths', 
        
             'maxCandidates', 
        
             'maxWorkUnits', 
        
             'maxWallClockMs' 
        
           ],

Whitelist workspace federation args for context-pack

context-pack now supports federated execution via workspace/workspaceId, but these flags are not included in wrapper validation, so pairofcleats context-pack --workspace ... is rejected as unknown flags. This makes the new federated path unreachable from the canonical CLI entrypoint.

Lines 766 to 770 in 3760d50

    
           'repo', 
        
           'seed', 
        
           'hops', 
        
           'maxTokens', 
        
           'maxBytes',

Whitelist repo-selection flags for federated context-pack

The federated implementation supports selection controls (select, repo-filter, includeDisabled, maxFederatedRepos), but none are accepted by the wrapper’s context-pack flag gate, so callers cannot constrain repo scope from pairofcleats context-pack. This blocks core federation controls and forces broad/default selection only.

Lines 876 to 882 in 3760d50

    
           if (sub === 'detect') { 
        
             validateArgs(rest, ['json', 'root', 'repo', 'languages'], ['root', 'repo', 'languages']); 
        
             return { script: 'tools/tooling/detect.js', extraArgs: [], args: rest }; 
        
           } 
        
           if (sub === 'install') { 
        
             validateArgs(rest, ['json', 'dry-run', 'no-fallback', 'root', 'repo', 'scope', 'languages', 'tools'], ['root', 'repo', 'scope', 'languages', 'tools']); 
        
             return { script: 'tools/tooling/install.js', extraArgs: [], args: rest };

Honor --root when resolving runtime env for tooling

New tooling subcommands accept --root, but wrapper-side runtime environment resolution still derives repo context from --repo only, so --root invocations can apply runtime settings from the wrong repo/cwd. This can misconfigure NODE_OPTIONS/threadpool shaping for tooling operations in multi-repo workflows.

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

doublemover · 2026-03-31T08:33:07Z

@codex please review again, find at least ten issues, do NOT look at yml/workflow files, look at code. Recent changes ideally. Prioritize P0/1's.

chatgpt-codex-connector · 2026-03-31T08:48:23Z

💡 Codex Review

Line 1094 in 339ff44

if (!job) return null;

Persist duplicate suppressions before returning no job

When claimNextJob suppresses queued duplicates during the scan, those mutations are only in memory until saveQueue runs. The early if (!job) return null exits before persisting the updated statuses or writing journal entries, so a pass that only suppresses duplicates silently loses the suppression work and leaves duplicates claimable on the next poll.

Lines 1065 to 1066 in 339ff44

    
           const runningDuplicate = findActiveDuplicateJob(queue.jobs, entry.idempotencyKey, entry.id); 
        
           if (runningDuplicate?.status === 'running') {

Check running duplicates explicitly before claiming work

findActiveDuplicateJob returns the first active match (queued or running), but this call only suppresses when the returned job is running. If a queued duplicate appears earlier in queue.jobs than a running duplicate with the same idempotency key, this branch misses the running duplicate and allows another claim, violating the duplicate-suppression intent.

PairOfCleats/tools/service/repair.js

Lines 1828 to 1833 in 339ff44

    
           const removedLogs = retentionPolicy.cleanupLogs === false 
        
             ? [] 
        
             : await pruneDirectoryArtifacts(logsDir, retainedArtifacts.keepLogs); 
        
           const removedReports = retentionPolicy.cleanupReports === false 
        
             ? [] 
        
             : await pruneDirectoryArtifacts(reportsDir, retainedArtifacts.keepReports);

Restrict compaction artifact deletion to the same queue

Compaction prunes logs/ and reports/ using a keep-set built from only the selected queue/quarantine records, but these directories are shared across queue names. Running compaction for one queue can therefore delete log/report files still referenced by jobs in other queues.

Lines 150 to 153 in 339ff44

    
           const [queue, quarantine] = await Promise.all([ 
        
             loadQueue(dirPath, queueName), 
        
             loadQuarantine(dirPath, queueName) 
        
           ]);

Compute orphan artifacts across all queue partitions

Orphan detection loads only one queue/quarantine partition before comparing against files in shared logs/ and reports/ directories. In multi-queue deployments this misclassifies artifacts belonging to other queues as orphans, and downstream cleanup can remove still-referenced files.

PairOfCleats/tools/service/queue-journal.js

Lines 86 to 87 in 339ff44

    
               .map((line) => JSON.parse(line)); 
        
           } catch {

Parse journal lines defensively instead of dropping all entries

A single malformed JSONL line currently throws inside .map(JSON.parse) and the outer catch returns [], discarding the entire journal payload. This turns partial log corruption into total state-loss for replay and diagnostics instead of isolating the bad line.

Lines 1623 to 1627 in 339ff44

    
           const { lockPath } = getQueuePaths(dirPath, queueName); 
        
           return withLock(lockPath, async () => { 
        
             await ensureQueueDir(dirPath); 
        
             const { logsDir, reportsDir } = await ensureJobDirs(dirPath); 
        
             const resolvedQueueName = resolveQueueName(queueName, null);

Resolve queue identity before selecting retry lock path

This function acquires a lock using the raw queueName and only then resolves the effective queue name. For aliases like auto, the lock file can differ from the queue file actually mutated, which allows concurrent writers to bypass mutual exclusion on the real queue state.

Lines 580 to 583 in 339ff44

    
           const logPath = normalizePathValue(job?.logPath); 
        
           const reportPath = normalizePathValue(job?.reportPath); 
        
           if (logPath) keepLogs.add(logPath); 
        
           if (reportPath) keepReports.add(reportPath);

Keep retained job artifacts when legacy paths are absent

Artifact retention only adds paths already present on job records. Retained legacy jobs without logPath/reportPath are not added to the keep-set, so compaction can delete their conventional artifacts even though the jobs themselves were retained.