Audit current structure cleanup#148
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request updates project documentation and configuration across multiple audit reports, module guidance, and a provider config setting. The audit documentation is substantially condensed from detailed findings to status summaries, while module responsibilities in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
docs/audits/08-feature-recommendations.md (1)
7-10: Minor readability polish for repeated sentence openings.Lines 7–10 start with the same verb pattern repeatedly; consider slight rewording to reduce repetition and improve scanability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audits/08-feature-recommendations.md` around lines 7 - 10, The four list items all start with the same verb "Add", making the list repetitive; rephrase each entry (e.g., use varied openings like "Provide", "Create", "Support", "Generate" or convert some into gerunds/imperatives) to improve readability while keeping the commands intact—specifically update the lines referencing `codex-export format=json`, the package-smoke script description (`npm pack` / temp project / ESM entry / installer dry-run), `codex-doctor exportPath=<file>`, and the current-structure docs generator so each line begins differently and scans better.docs/audits/10-testing-gaps.md (1)
7-11: Optional: Vary sentence structure for readability.Four consecutive bullets begin with the same pattern ("test/X covers..."). While clear, varying the structure could improve flow.
✨ Suggested rewording
Current high-value coverage: -- `test/index.test.ts` covers tool registration and extracted tool behavior through plugin wiring. -- `test/tools-codex-*.test.ts` covers focused tool regressions. -- `test/storage.test.ts` covers non-destructive export defaults. -- `test/doc-parity.test.ts` now covers config contract, tool registry count, and stale audit anchors. +- Tool registration and plugin wiring: `test/index.test.ts` +- Focused tool regressions: `test/tools-codex-*.test.ts` +- Non-destructive export defaults: `test/storage.test.ts` +- Config contract, tool registry count, and stale audit anchors: `test/doc-parity.test.ts` - `test/contracts/*` pins Codex response shapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audits/10-testing-gaps.md` around lines 7 - 11, Rewrite the consecutive bullet lines so they vary sentence structure while preserving meaning; instead of repeating "test/... covers..." start some bullets with verbs or clauses (e.g., "Tool registration and extracted tool behavior are exercised by test/index.test.ts", "Focused regressions are validated in test/tools-codex-*.test.ts", "Non-destructive export defaults are checked in test/storage.test.ts", "test/doc-parity.test.ts now asserts the config contract, tool registry count, and stale audit anchors", and "Response shapes are pinned in test/contracts/*") — update the bullets accordingly to improve flow while keeping the same file references and intent.docs/audits/_findings/T06-filesystem.md (1)
11-15: Optional: Vary sentence structure.Five consecutive bullets use "owns" to describe module responsibilities. Consider varying the phrasing for better readability.
✨ Suggested rewording
Current filesystem ownership: -- `lib/storage/paths.ts` resolves config roots. -- `lib/storage/load-save.ts` owns account load/save and transactions. -- `lib/storage/atomic-write.ts` owns atomic write primitives. -- `lib/storage/worktree-lock.ts` owns lock behavior. -- `lib/storage/export-import.ts` owns import/export safety. +- Config root resolution: `lib/storage/paths.ts` +- Account load/save and transactions: `lib/storage/load-save.ts` +- Atomic write primitives: `lib/storage/atomic-write.ts` +- Lock behavior: `lib/storage/worktree-lock.ts` +- Import/export safety: `lib/storage/export-import.ts`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audits/_findings/T06-filesystem.md` around lines 11 - 15, The five consecutive bullets all use the verb "owns," which is repetitive; edit the bullets for lib/storage/paths.ts, lib/storage/load-save.ts, lib/storage/atomic-write.ts, lib/storage/worktree-lock.ts, and lib/storage/export-import.ts to use varied verbs that reflect each module's responsibility (e.g., "resolves" or "calculates" for paths, "handles" or "manages" for load-save and transactions, "implements" or "provides" for atomic-write, "controls" or "coordinates" for worktree-lock, and "ensures" or "validates" for export-import) so the list reads more naturally while preserving the original meaning.docs/audits/_meta/dedup-report.md (1)
7-10: Optional: Vary bullet structure.Three consecutive bullets begin with "Collapse" and end with "resolved ... row." Consider varying the phrasing.
✨ Suggested rewording
Deduplication policy: - Prefer current-source findings over historical claims. -- Collapse repeated old monolith claims into one resolved architecture row. -- Collapse destructive-export claims into one resolved tool/storage row. -- Collapse config continuity claims into one resolved config row. +- Repeated old monolith claims → single resolved architecture row. +- Destructive-export claims → single resolved tool/storage row. +- Config continuity claims → single resolved config row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audits/_meta/dedup-report.md` around lines 7 - 10, The three consecutive bullets that all start with "Collapse" and end with "resolved ... row" (specifically the lines containing "Collapse repeated old monolith claims into one resolved architecture row.", "Collapse destructive-export claims into one resolved tool/storage row.", and "Collapse config continuity claims into one resolved config row.") are repetitive; rewrite them to vary sentence structure while preserving meaning—for example, keep the first as-is, change the others to use different verbs or passive constructions such as "Aggregate repeated destructive-export claims into a single resolved tool/storage row" and "Consolidate config continuity claims into one resolved config row" or similar phrasing, so each bullet starts differently and avoids the identical "Collapse ... resolved ... row" pattern.docs/audits/_findings/T16-code-health.md (1)
11-15: Optional: Vary sentence structure.Four bullets use "is" to describe module characteristics. Consider varying the structure for better flow.
✨ Suggested rewording
Current code-health evidence: -- `index.ts` is 3694 lines but no longer owns inline tool bodies. -- `lib/storage.ts` is a 79-line facade. -- `lib/accounts.ts` is a 366-line facade/orchestrator. +- `index.ts`: 3694 lines (no longer owns inline tool bodies) +- `lib/storage.ts`: 79-line facade +- `lib/accounts.ts`: 366-line facade/orchestrator - `lib/tools/index.ts` maps exactly 21 tool factories. - Largest remaining concentration is the request layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audits/_findings/T16-code-health.md` around lines 11 - 15, The four bullets currently all use the same "is" structure (e.g. "`index.ts` is 3694 lines", "`lib/storage.ts` is a 79-line facade.", "`lib/accounts.ts` is a 366-line facade/orchestrator.", "`lib/tools/index.ts` maps exactly 21 tool factories.")—please vary sentence structure for better flow by rephrasing at least two bullets (for example: "index.ts spans 3694 lines", "lib/storage.ts serves as a 79-line facade", "lib/accounts.ts functions as a 366-line orchestrator/facade", or "lib/tools/index.ts maps 21 tool factories") while preserving the original facts and conciseness.docs/audits/_findings/T02-security.md (1)
19-19: Consider clarifying the keychain opt-in model.The phrasing "JSON storage remains available for users who do not opt into the native keychain" is technically correct but could be clearer. Based on the code snippets showing
lib/storage/keychain.tsimplementation, JSON is the default storage backend, and keychain is an opt-in enhancement (viaCODEX_KEYCHAIN=1).Consider rephrasing to: "JSON storage is the default backend; users may opt into native keychain via
CODEX_KEYCHAIN=1for enhanced security."This makes the opt-in nature and default behavior clearer without changing the technical accuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audits/_findings/T02-security.md` at line 19, Update the sentence to clearly state that JSON is the default and the native keychain is opt-in: reword the line to something like "JSON storage is the default backend; users may opt into native keychain via CODEX_KEYCHAIN=1 for enhanced security." Reference the keychain implementation (lib/storage/keychain.ts) and the environment flag (CODEX_KEYCHAIN) to ensure the phrasing matches the actual opt-in behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 13: The README line claiming a "7-step fetch pipeline" is inaccurate;
update the text referencing index.ts (the plugin entry) to reflect the actual
implementation by either changing "7-step fetch pipeline" to "2-step fetch
pipeline" (matching the explicit Step 1 and Step 3 labels in index.ts) or,
preferably, to "5-stage fetch pipeline" and list the five implemented operations
(URL rewriting, request body transformation, OAuth header injection, SSE
conversion, and error handling) so the documentation matches the
symbols/behavior in index.ts.
In `@docs/audits/_findings/T04-request-pipeline.md`:
- Around line 11-14: Update the documentation to explicitly state that
transformRequestBody also enforces stream=true in addition to store: false; edit
the T04-request-pipeline.md content where the contract bullets are listed to add
a line or amend the existing bullet referencing transformRequestBody so it reads
that transformRequestBody forces both store: false and stream: true, and include
a brief note linking this behavior to the request pipeline/components that rely
on it (e.g., transformRequestBody and any callers in the request pipeline).
In `@docs/audits/_findings/T10-error-handling.md`:
- Line 11: The schema in lib/tools/codex-help.ts advertises a "metrics" topic
but the sections array lacks a section with key "metrics", so update codex-help
to either add a corresponding help section object (with key "metrics" and the
appropriate title/content) into the sections array or remove "metrics" from the
advertised topics in the exported schema; locate the schema and sections array
in lib/tools/codex-help.ts (the exported help schema/array used by codex-help)
and make the change so advertised topics and actual section keys stay in sync.
In `@docs/audits/11-dx-cli-docs.md`:
- Line 7: Update the codex-help schema description to stop advertising the
nonexistent "metrics" topic by removing "metrics" from the topics list string
(the literal: "Optional topic: setup, switch, health, backup, dashboard,
metrics.") so it reads without "metrics"; modify the line in
docs/audits/11-dx-cli-docs.md that contains that schema description to reflect
the corrected topics list for the codex-help entry.
---
Nitpick comments:
In `@docs/audits/_findings/T02-security.md`:
- Line 19: Update the sentence to clearly state that JSON is the default and the
native keychain is opt-in: reword the line to something like "JSON storage is
the default backend; users may opt into native keychain via CODEX_KEYCHAIN=1 for
enhanced security." Reference the keychain implementation
(lib/storage/keychain.ts) and the environment flag (CODEX_KEYCHAIN) to ensure
the phrasing matches the actual opt-in behavior.
In `@docs/audits/_findings/T06-filesystem.md`:
- Around line 11-15: The five consecutive bullets all use the verb "owns," which
is repetitive; edit the bullets for lib/storage/paths.ts,
lib/storage/load-save.ts, lib/storage/atomic-write.ts,
lib/storage/worktree-lock.ts, and lib/storage/export-import.ts to use varied
verbs that reflect each module's responsibility (e.g., "resolves" or
"calculates" for paths, "handles" or "manages" for load-save and transactions,
"implements" or "provides" for atomic-write, "controls" or "coordinates" for
worktree-lock, and "ensures" or "validates" for export-import) so the list reads
more naturally while preserving the original meaning.
In `@docs/audits/_findings/T16-code-health.md`:
- Around line 11-15: The four bullets currently all use the same "is" structure
(e.g. "`index.ts` is 3694 lines", "`lib/storage.ts` is a 79-line facade.",
"`lib/accounts.ts` is a 366-line facade/orchestrator.", "`lib/tools/index.ts`
maps exactly 21 tool factories.")—please vary sentence structure for better flow
by rephrasing at least two bullets (for example: "index.ts spans 3694 lines",
"lib/storage.ts serves as a 79-line facade", "lib/accounts.ts functions as a
366-line orchestrator/facade", or "lib/tools/index.ts maps 21 tool factories")
while preserving the original facts and conciseness.
In `@docs/audits/_meta/dedup-report.md`:
- Around line 7-10: The three consecutive bullets that all start with "Collapse"
and end with "resolved ... row" (specifically the lines containing "Collapse
repeated old monolith claims into one resolved architecture row.", "Collapse
destructive-export claims into one resolved tool/storage row.", and "Collapse
config continuity claims into one resolved config row.") are repetitive; rewrite
them to vary sentence structure while preserving meaning—for example, keep the
first as-is, change the others to use different verbs or passive constructions
such as "Aggregate repeated destructive-export claims into a single resolved
tool/storage row" and "Consolidate config continuity claims into one resolved
config row" or similar phrasing, so each bullet starts differently and avoids
the identical "Collapse ... resolved ... row" pattern.
In `@docs/audits/08-feature-recommendations.md`:
- Around line 7-10: The four list items all start with the same verb "Add",
making the list repetitive; rephrase each entry (e.g., use varied openings like
"Provide", "Create", "Support", "Generate" or convert some into
gerunds/imperatives) to improve readability while keeping the commands
intact—specifically update the lines referencing `codex-export format=json`, the
package-smoke script description (`npm pack` / temp project / ESM entry /
installer dry-run), `codex-doctor exportPath=<file>`, and the current-structure
docs generator so each line begins differently and scans better.
In `@docs/audits/10-testing-gaps.md`:
- Around line 7-11: Rewrite the consecutive bullet lines so they vary sentence
structure while preserving meaning; instead of repeating "test/... covers..."
start some bullets with verbs or clauses (e.g., "Tool registration and extracted
tool behavior are exercised by test/index.test.ts", "Focused regressions are
validated in test/tools-codex-*.test.ts", "Non-destructive export defaults are
checked in test/storage.test.ts", "test/doc-parity.test.ts now asserts the
config contract, tool registry count, and stale audit anchors", and "Response
shapes are pinned in test/contracts/*") — update the bullets accordingly to
improve flow while keeping the same file references and intent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df7ffce5-7690-42b0-b484-d316750b4988
⛔ Files ignored due to path filters (2)
docs/audits/_meta/findings-ledger.csvis excluded by!**/*.csvdocs/audits/_meta/sha.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
AGENTS.mdconfig/minimal-opencode.jsondocs/DOCUMENTATION.mddocs/audits/01-executive-summary.mddocs/audits/02-system-map.mddocs/audits/03-critical-issues.mddocs/audits/04-high-priority.mddocs/audits/05-medium.mddocs/audits/06-low-priority.mddocs/audits/07-refactoring-plan.mddocs/audits/08-feature-recommendations.mddocs/audits/09-security-trust.mddocs/audits/10-testing-gaps.mddocs/audits/11-dx-cli-docs.mddocs/audits/12-quick-wins.mddocs/audits/13-phased-roadmap.mddocs/audits/14-top20.mddocs/audits/15-file-by-file.mddocs/audits/16-verdict.mddocs/audits/INDEX.mddocs/audits/_findings/T01-architecture.mddocs/audits/_findings/T02-security.mddocs/audits/_findings/T03-rotation.mddocs/audits/_findings/T04-request-pipeline.mddocs/audits/_findings/T05-type-safety.mddocs/audits/_findings/T06-filesystem.mddocs/audits/_findings/T07-concurrency.mddocs/audits/_findings/T08-performance.mddocs/audits/_findings/T09-observability.mddocs/audits/_findings/T10-error-handling.mddocs/audits/_findings/T11-config-installer.mddocs/audits/_findings/T12-cli-ui.mddocs/audits/_findings/T13-test-coverage.mddocs/audits/_findings/T14-dependencies.mddocs/audits/_findings/T15-ci-release.mddocs/audits/_findings/T16-code-health.mddocs/audits/_meta/AUDIT-RUBRIC.mddocs/audits/_meta/dedup-report.mddocs/audits/_meta/environment.mddocs/audits/_meta/f1-post-merge-review.mddocs/audits/_meta/oracle-review.mddocs/audits/_meta/scope-whitelist.txtdocs/audits/_meta/severity-reclassifications.mddocs/audits/_meta/verification-report.mddocs/development/ARCHITECTURE.mddocs/development/TESTING.mddocs/troubleshooting.mdlib/AGENTS.mdlib/runtime.tslib/tools/AGENTS.mdlib/tools/_shared.tslib/tools/codex-export.tslib/tools/codex-help.tslib/tools/codex-remove.tslib/tools/index.tsscripts/install-oc-codex-multi-auth-core.jsscripts/install-oc-codex-multi-auth.jstest/AGENTS.mdtest/README.mdtest/doc-parity.test.tstest/index.test.tstest/install-oc-codex-multi-auth.test.tsvitest.config.ts
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
1 participant
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.
Summary
Testing
Compliance Confirmation
Notes
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
this pr regenerates the full audit corpus against the current source sha and fixes six specific validation findings: the
metricstopic mismatch incodex-help, theforce=trueoverwrite default incodex-export, missingconfirm=trueincodex-removeguidance,reasoning.encrypted_contentin minimal config, the installer direct-run path detection refactor (logic now lives in the core module), and coverage threshold calibration to the measured baseline. docs, tests, and ci are all aligned post-split with the 21-tool registry.Confidence Score: 5/5
safe to merge — all p1 findings from prior review resolved, only p2 style notes remain
no p0 or p1 findings in this review. the previous p1 (metrics topic mismatch) is definitively fixed. the
force=falseflip andconfirm=trueguidance fixes are correct and safer. two p2 notes: theindex.tsbranch threshold drop to 50% is acknowledged and tracked in #149, and the direct-run guard inclean-dist.jsomitsrealpathSync. neither blocks merge.vitest.config.ts — the
index.ts50% branch floor is the lowest threshold in the repo for a production file; worth revisiting once #149 landsImportant Files Changed
metricsfrom advertised topics and tightened filter to strict===; all sections now match the schema description exactlyforcedefault flipped fromtruetofalse, preventing silent overwrites on windows where file-lock races (EPERM) could corrupt backupsconfirm=truematching the tool's required argumentrelative()guard ensuring onlydist/is deleted; windows-aware path normalization is correct but the direct-run guard skipsrealpathSyncunlike the installernormalizePathForCompareandisDirectRunPathmoved here from the wrapper; added!modulePathnull guard; windows lowercase normalization preserved correctlyresolveRealPathnow correctly passesundefinedto core which has its ownrealpathSyncdefaultindex.tsbranches lowered 70→50% to match measured baseline; tracked in #149 but the entrypoint floor is notably lowruntime.ts; tools now import directly from sourceToolContextdoc aligned with current registrycleanDist()include: ["reasoning.encrypted_content"]to the openai provider options, aligning with the stateless contract tested indoc-parity.test.tsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["npm run build"] --> B["node scripts/clean-dist.js"] B --> C{"relative(repoRoot, distDir) === 'dist'?"} C -- "no" --> D["throw: Refusing to clean non-dist path"] C -- "yes" --> E["fs.rm(dist/, recursive, force)"] E --> F["tsc"] F --> G["node scripts/copy-oauth-success.js"] G --> H["dist/ ready"] subgraph "codex-help fix" I["topic param"] --> J{"normalizedTopic.length === 0?"} J -- "yes" --> K["show all sections"] J -- "no" --> L{"section.key === normalizedTopic"} L -- "match" --> M["show section"] L -- "no match" --> N["Unknown topic error"] end subgraph "codex-export fix" O["force param"] --> P{"force ?? false"} P -- "false (default)" --> Q["exportAccounts safe-write"] P -- "true (explicit)" --> R["exportAccounts overwrite"] endComments Outside Diff (2)
lib/tools/codex-help.ts, line 22-25 (link)metricstopic advertised but never definedthe schema description lists
"metrics"as a valid topic, but no section withkey: "metrics"exists in thesectionsarray (onlysetup,switch,health,dashboard,backup). before this PR the substring filter would just return no results silently; now that the filter uses strict===, any caller (llm, user, or automated agent) that passestopic: "metrics"will immediately get"Unknown topic: metrics"— breaking the documented contract. either add the section or dropmetricsfrom the description.Prompt To Fix With AI
lib/tools/codex-help.ts, line 24 (link)metricstopic advertised but never definedthe schema description still lists
"metrics"as a valid topic, but no section withkey: "metrics"exists in thesectionsarray (onlysetup,switch,health,dashboard,backup). now that the filter was tightened to strict===in this pr, any caller that passestopic: "metrics"will immediately receive"Unknown topic: metrics"— breaking the documented contract. the previousincludesbehaviour would silently return nothing, but the new strict filter makes the mismatch a hard, visible failure. either add the missing section or dropmetricsfrom the description.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "docs: track coverage follow-up" | Re-trigger Greptile