fix: resolve fresh-clone test failure, 236 lint warnings, and dead code#603
Open
alessandrovarela wants to merge 2 commits intoSynkraAI:mainfrom
Open
fix: resolve fresh-clone test failure, 236 lint warnings, and dead code#603alessandrovarela wants to merge 2 commits intoSynkraAI:mainfrom
alessandrovarela wants to merge 2 commits intoSynkraAI:mainfrom
Conversation
…de [Story CONTRIB-1] - Fix _timeAgo test bug: replace setDate(getDate()-7) with exact ms arithmetic to prevent flaky 6d/7d results across DST/month boundaries - Auto-fix 222 ESLint warnings (trailing commas, single quotes) - Fix 14 manual ESLint warnings: remove dead imports (execSync, fs/path/yaml), remove unused variables (fileCount, present), prefix intentional unused params with _ convention, wrap default case in braces - Remove redundant ensureGitignore() from session-manager.js — .synapse/ subdirs already covered by root .gitignore - Sync package-lock.json version field (5.0.2 → 5.0.3) No application behavior changed. All fixes are test/lint/dead-code only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alessandrovarela
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@alessandrovarela is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
area: agents
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR applies widespread non-functional edits: formatting (trailing commas, quote normalization), removal of unused imports/variables, small parameter/catch renames, updates to manifest hashes/timestamps, and deletion of the ensureGitignore routine and its tests. No new features or major control-flow changes. Changes
Sequence Diagram(s)(omitted — changes are non-functional/formatting and do not introduce new multi-component control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Welcome to aiox-core! Thanks for your first pull request.
What happens next?
- Automated checks will run on your PR
- A maintainer will review your changes
- Once approved, we'll merge your contribution!
PR Checklist:
- Tests pass (
npm test) - Linting passes (
npm run lint) - Commit messages follow Conventional Commits
Thanks for contributing!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/unit/tok5-analytics.test.js (1)
11-18: Use absolute imports for script modules in this test fileLine 18 and Line 26 still use relative
require(...)paths. Please switch these to the project’s absolute import style to match repo standards.Suggested refactor
const { createEvent, sanitizeEvent, validateEvent, pruneOldEntries, generateSampleData, RETENTION_DAYS, -} = require('../../.aiox-core/infrastructure/scripts/collect-tool-usage'); +} = require('.aiox-core/infrastructure/scripts/collect-tool-usage'); const { compareBaseline, generateRecommendations, generateReport, aggregateUsage, DEFAULT_THRESHOLDS, -} = require('../../.aiox-core/infrastructure/scripts/generate-optimization-report'); +} = require('.aiox-core/infrastructure/scripts/generate-optimization-report');As per coding guidelines, "
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code."Also applies to: 20-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/tok5-analytics.test.js` around lines 11 - 18, The test file imports scripts using relative require paths; update the two relative requires (the one importing createEvent, sanitizeEvent, validateEvent, pruneOldEntries, generateSampleData, RETENTION_DAYS and the other require at the later import) to use the project’s absolute import style instead of '../../.aiox-core/...' so they match repo standards; locate the import that provides createEvent/sanitizeEvent/validateEvent/pruneOldEntries/generateSampleData/RETENTION_DAYS and the other import around line 26 and replace their require(...) calls with the equivalent absolute module specifiers used across the codebase while keeping the same imported symbols and names.squads/claude-code-mastery/scripts/validate-setup.js (1)
38-48: Consider: Empty catch blocks silently discard error details.The empty
catchblocks returnfalseon any parsing error, which is acceptable for a pass/fail validation script. However, if debugging becomes necessary in the future, adding optional verbose logging could be helpful.This is a minor observation and not blocking—the current behavior is reasonable for the script's purpose.
Also applies to: 68-77, 82-93, 111-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@squads/claude-code-mastery/scripts/validate-setup.js` around lines 38 - 48, The catch blocks in the validation functions (e.g., the check arrow function that reads settingsPath and parses JSON via fs.readFileSync/JSON.parse to compute deny) swallow errors silently; update each catch to optionally log the caught error when verbose debugging is enabled (for example check process.env.VERBOSE or a passed verbose flag) so normal behavior still returns false but developers can enable detailed logs for debugging; apply the same change to the other similar validation blocks mentioned (the ones that read/parse files and return false on error).tests/synapse/hook-runtime.test.js (1)
72-74: Remove stale local flag; it is unused in the assertion path.Line 73 introduces
cleanupCalled, but the test validates cleanup throughsm._wasCalled(). Keeping both mechanisms adds noise without value.Proposed cleanup
- // Mock session-manager with prompt_count: 0 and trackable cleanStaleSessions - const cleanupCalled = false; + // Mock session-manager with prompt_count: 0 and trackable cleanStaleSessions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/synapse/hook-runtime.test.js` around lines 72 - 74, The test introduces an unused local flag cleanupCalled which is never checked; remove the declaration and any assignments to cleanupCalled and rely solely on the session-manager mock's verification method (sm._wasCalled()) to assert cleanup behavior—edit the test around the writeFile block and any nearby references to cleanupCalled so only sm._wasCalled() is used for the assertion.tests/synapse/session-manager.test.js (1)
317-317: Align section/docs numbering after gitignore test removal.Line 317 updates one section label, but the surrounding headers/comments are now inconsistent (e.g., later section numbers skip, top banner still mentions gitignore tests). Consider a quick docs-only pass to keep the test file structure coherent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/synapse/session-manager.test.js` at line 317, Update the test file's section headers and top banner so numbering and references are consistent after removal of the gitignore test: search for the comment "Session Continuity (AC: 6)" and any surrounding numbered section labels, update them to follow the new sequential order, and also update the top banner/text that still mentions gitignore tests so all headings, section numbers and comments align across the file (ensure any cross-references or later section numbers that currently skip are renumbered to be continuous).packages/installer/tests/unit/merger/yaml-merger.test.js (1)
17-19: Use the repository’s absolute import style for these touched requires.The comma-only edits are harmless, but these hunks still resolve modules through
__dirname+..traversal. Since those imports were touched here, this is a good spot to bring them in line with the project import convention. As per coding guidelines,**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.Also applies to: 56-58, 66-68, 74-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/installer/tests/unit/merger/yaml-merger.test.js` around lines 17 - 19, The test currently requires modules using path.join(__dirname, '..'...) (e.g. the YamlMerger require that imports YamlMerger) which violates the repo's absolute-import convention; replace each require(path.join(__dirname, ...)) in this test (the YamlMerger import and the other touched requires at the same file) with the project's absolute import style (use the module specifiers the project uses for source imports) so the test imports YamlMerger and the related helpers/fixtures via absolute require/imports instead of __dirname + .. traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/installer/tests/unit/ide-sync-integration/ide-sync-integration.test.js`:
- Line 37: The test contains a double-quoted string for the require assertion;
update the assertion literal to use single quotes to match project style —
change the line containing "const { commandSync, commandValidate } =
require('../../../../.aiox-core/infrastructure/scripts/ide-sync/index')" so the
require string uses single quotes (refer to the constants commandSync and
commandValidate to locate the line) and run the unit tests/linter to confirm
style compliance.
In `@scripts/package-synapse.js`:
- Line 226: The generated JSON "command" entry currently uses unquoted
$CLAUDE_PROJECT_DIR which will break if the path contains spaces; update the
"command" value (the "command" JSON key that references $CLAUDE_PROJECT_DIR and
.claude/hooks/synapse-engine.cjs) to wrap the full path in quotes so the shell
receives a single argument (e.g., ensure the value is "node
\"$CLAUDE_PROJECT_DIR/.claude/hooks/synapse-engine.cjs\"" or otherwise properly
escapes quotes), preserving correct escaping for JSON strings.
In `@tests/ids/layer-classifier.test.js`:
- Around line 7-9: Replace the relative require that pulls classifyLayer and
LAYER_RULES using path.resolve('../../...') with an absolute repo-root import;
update the require for the module that exports classifyLayer and LAYER_RULES to
use the project-root absolute path (the module that provides classifyLayer and
LAYER_RULES in the layer-classifier file) so the test no longer relies on
`../../` traversal and follows the absolute-import guideline.
---
Nitpick comments:
In `@packages/installer/tests/unit/merger/yaml-merger.test.js`:
- Around line 17-19: The test currently requires modules using
path.join(__dirname, '..'...) (e.g. the YamlMerger require that imports
YamlMerger) which violates the repo's absolute-import convention; replace each
require(path.join(__dirname, ...)) in this test (the YamlMerger import and the
other touched requires at the same file) with the project's absolute import
style (use the module specifiers the project uses for source imports) so the
test imports YamlMerger and the related helpers/fixtures via absolute
require/imports instead of __dirname + .. traversal.
In `@squads/claude-code-mastery/scripts/validate-setup.js`:
- Around line 38-48: The catch blocks in the validation functions (e.g., the
check arrow function that reads settingsPath and parses JSON via
fs.readFileSync/JSON.parse to compute deny) swallow errors silently; update each
catch to optionally log the caught error when verbose debugging is enabled (for
example check process.env.VERBOSE or a passed verbose flag) so normal behavior
still returns false but developers can enable detailed logs for debugging; apply
the same change to the other similar validation blocks mentioned (the ones that
read/parse files and return false on error).
In `@tests/synapse/hook-runtime.test.js`:
- Around line 72-74: The test introduces an unused local flag cleanupCalled
which is never checked; remove the declaration and any assignments to
cleanupCalled and rely solely on the session-manager mock's verification method
(sm._wasCalled()) to assert cleanup behavior—edit the test around the writeFile
block and any nearby references to cleanupCalled so only sm._wasCalled() is used
for the assertion.
In `@tests/synapse/session-manager.test.js`:
- Line 317: Update the test file's section headers and top banner so numbering
and references are consistent after removal of the gitignore test: search for
the comment "Session Continuity (AC: 6)" and any surrounding numbered section
labels, update them to follow the new sequential order, and also update the top
banner/text that still mentions gitignore tests so all headings, section numbers
and comments align across the file (ensure any cross-references or later section
numbers that currently skip are renumbered to be continuous).
In `@tests/unit/tok5-analytics.test.js`:
- Around line 11-18: The test file imports scripts using relative require paths;
update the two relative requires (the one importing createEvent, sanitizeEvent,
validateEvent, pruneOldEntries, generateSampleData, RETENTION_DAYS and the other
require at the later import) to use the project’s absolute import style instead
of '../../.aiox-core/...' so they match repo standards; locate the import that
provides
createEvent/sanitizeEvent/validateEvent/pruneOldEntries/generateSampleData/RETENTION_DAYS
and the other import around line 26 and replace their require(...) calls with
the equivalent absolute module specifiers used across the codebase while keeping
the same imported symbols and names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8857d435-b6ac-4309-a078-d55ee9109841
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
.aiox-core/core/code-intel/helpers/dev-helper.js.aiox-core/core/code-intel/helpers/devops-helper.js.aiox-core/core/code-intel/helpers/planning-helper.js.aiox-core/core/code-intel/helpers/qa-helper.js.aiox-core/core/config/template-overrides.js.aiox-core/core/doctor/checks/rules-files.js.aiox-core/core/graph-dashboard/cli.js.aiox-core/core/graph-dashboard/data-sources/code-intel-source.js.aiox-core/core/ids/layer-classifier.js.aiox-core/core/synapse/layers/layer-processor.js.aiox-core/core/synapse/session/session-manager.js.aiox-core/data/capability-detection.js.aiox-core/data/entity-registry.yaml.aiox-core/data/tok3-token-comparison.js.aiox-core/data/tool-search-validation.js.aiox-core/install-manifest.yaml.aiox-core/utils/filters/index.js.claude/hooks/precompact-session-digest.cjsbin/aiox.jsbin/utils/framework-guard.jspackages/installer/src/installer/brownfield-upgrader.jspackages/installer/src/pro/pro-scaffolder.jspackages/installer/src/wizard/ide-config-generator.jspackages/installer/src/wizard/index.jspackages/installer/tests/unit/artifact-copy-pipeline/artifact-copy-pipeline.test.jspackages/installer/tests/unit/claude-md-template-v5/claude-md-template-v5.test.jspackages/installer/tests/unit/entity-registry-bootstrap.test.jspackages/installer/tests/unit/generate-settings-json/generate-settings-json.test.jspackages/installer/tests/unit/ide-sync-integration/ide-sync-integration.test.jspackages/installer/tests/unit/merger/yaml-merger.test.jsscripts/package-synapse.jsscripts/validate-package-completeness.jssquads/claude-code-mastery/scripts/validate-setup.jstests/cli/validate-publish.test.jstests/code-intel/creation-helper.test.jstests/code-intel/dev-helper.test.jstests/code-intel/registry-syncer.test.jstests/config/schema-validation.test.jstests/config/template-overrides.test.jstests/core/health-check/base-check.test.jstests/core/health-check/check-registry.test.jstests/core/registry/build-registry.test.jstests/graph-dashboard/code-intel-source.test.jstests/graph-dashboard/html-formatter.test.jstests/graph-dashboard/stats-renderer.test.jstests/graph-dashboard/tree-renderer.test.jstests/ids/layer-classifier.test.jstests/ids/layer-integration.test.jstests/installer/pro-scaffolder.test.jstests/integration/code-intel/helpers-with-registry.test.jstests/memory/memory-format.test.jstests/pro-recover.test.jstests/synapse/benchmarks/wave6-journey.jstests/synapse/bridge/uap-session-bridge.test.jstests/synapse/diagnostics/collectors.test.jstests/synapse/e2e/pipeline-audit.e2e.jstests/synapse/hook-runtime.test.jstests/synapse/session-manager.test.jstests/unit/tok5-analytics.test.js
💤 Files with no reviewable changes (5)
- .aiox-core/core/doctor/checks/rules-files.js
- .aiox-core/core/synapse/session/session-manager.js
- .aiox-core/core/code-intel/helpers/devops-helper.js
- .aiox-core/data/tok3-token-comparison.js
- bin/aiox.js
packages/installer/tests/unit/ide-sync-integration/ide-sync-integration.test.js
Outdated
Show resolved
Hide resolved
Comment on lines
7
to
9
| const { classifyLayer, LAYER_RULES } = require( | ||
| path.resolve(__dirname, '../../.aiox-core/core/ids/layer-classifier') | ||
| path.resolve(__dirname, '../../.aiox-core/core/ids/layer-classifier'), | ||
| ); |
There was a problem hiding this comment.
Switch this require to a repo-root absolute path.
This still depends on ../../ traversal. Please use a path rooted at the project root to align with the import rule.
Proposed change
const { classifyLayer, LAYER_RULES } = require(
- path.resolve(__dirname, '../../.aiox-core/core/ids/layer-classifier'),
+ path.join(process.cwd(), '.aiox-core/core/ids/layer-classifier'),
);As per coding guidelines, **/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ids/layer-classifier.test.js` around lines 7 - 9, Replace the relative
require that pulls classifyLayer and LAYER_RULES using path.resolve('../../...')
with an absolute repo-root import; update the require for the module that
exports classifyLayer and LAYER_RULES to use the project-root absolute path (the
module that provides classifyLayer and LAYER_RULES in the layer-classifier file)
so the test no longer relies on `../../` traversal and follows the
absolute-import guideline.
- Use single quotes in ide-sync-integration test assertion (CR review SynkraAI#1) - Restore quotes around $CLAUDE_PROJECT_DIR in package-synapse.js template to prevent shell word-splitting on paths with spaces (CR review SynkraAI#2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Contributor
|
PR muito bem estruturada. 30 arquivos tocados mas com escopo claro e disciplinado — nenhuma mudança de comportamento. Observações:
Ótima contribuição pra experiência de onboarding do projeto. |
Author
Fala Nikolas, obrigado pelo feedback, principalmente vindo de você que é bem atuante e um colaborador experiente do projeto. |
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
2 participants
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
Community contribution fixing 3 issues that affect fresh-clone contributor experience:
_timeAgoinstats-renderer.test.jsusedsetDate(getDate()-7)which doesn't guarantee exactly 7 days in milliseconds, causing flaky 6d/7d results across DST/month boundaries. Replaced with exact ms arithmetic.ensureGitignore()insession-manager.jscreated a redundant.synapse/.gitignore— the root.gitignorealready covers.synapse/sessions/,.synapse/cache/,.synapse/metrics/. Removed function, call, export, and 2 related tests.No application behavior changed. All fixes are test/lint/dead-code only.
Story Reference
CONTRIB-1: Fresh-Clone Quality Fix
Acceptance Criteria
npm testpasses — 0 real test failures (stats-renderer fixed)npm run lint— 0 errors, 0 warnings (was 236 warnings)npm run typecheck— 0 errorsensureGitignore()removed with no regressionTest Plan
npm teston Node 18, 20, 22, 24, 25 matrixnpm run lint— must exit 0npm run typecheck— must exit 0stats-renderer.test.js_timeAgo test passes consistentlysession-manager.test.jspasses without ensureGitignore tests🤖 Generated with Claude Code
Summary by CodeRabbit