feat: add application learning queue#890
Conversation
github-actions
Bot
added
📄 docs
🔴 core-architecture
⚠️ agent-behavior
🔧 scripts
📦 dependencies
labels
|
Warning Review limit reached
More reviews will be available in 1 minute and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Markdown-backed learning queue and a Node ESM CLI ( ChangesLearning Queue Feature
Sequence DiagramsequenceDiagram
participant CLI as CLI User
participant Main as main()
participant Queue as learning-queue.mjs
participant File as data/learning-queue.md
CLI->>Main: learning-queue add --source --target --feedback --proposal
Main->>Queue: addEntry(...)
Queue->>File: append rendered entry
Queue-->>Main: return entry object
Main-->>CLI: output JSON result
CLI->>Main: learning-queue set-status --id --status
Main->>Queue: setStatus(...)
Queue->>File: regex update **Status:**
Queue-->>Main: success/error
Main-->>CLI: output JSON result
CLI->>Main: learning-queue list [--status filter]
Main->>Queue: listEntries(...)
Queue->>File: parseQueue()
Queue-->>Main: filtered entries
Main-->>CLI: output JSON result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@learning-queue.mjs`:
- Around line 121-122: The code currently calls readFileSync(queuePath, 'utf-8')
which will throw a raw ENOENT if the queue file is missing; before calling
readFileSync in this block, check for the file's existence (e.g., fs.existsSync
or a try/catch around readFileSync) and when missing throw or return a clear
domain-specific error (e.g., a QueueNotFoundError or an Error with message
"queue file not found for queuePath") so callers get a graceful, descriptive
failure instead of a raw FS ENOENT; update the handling around the const content
= readFileSync(queuePath, 'utf-8') line and ensure any downstream use of
content/next accounts for this error path.
- Around line 76-93: The renderEntry function currently injects entry.source,
entry.feedback, and entry.proposal verbatim into markdown; add
validation/sanitization before rendering (in renderEntry and the analogous
function handling entries at lines ~95-113) to either escape or reject unsafe
content: check entry.proposal, entry.source, and entry.feedback for markdown
fence markers like "```" and for unexpected newlines in fields that must be
single-line, and if found throw a descriptive Error (e.g., "--proposal cannot
contain ``` fence markers") or replace/escape backticks/newlines safely so the
markdown structure cannot be broken.
- Around line 117-124: The setStatus function builds a RegExp with unescaped CLI
id which allows regex injection; sanitize or escape the id before using it in
new RegExp (or avoid RegExp entirely by parsing the file and matching entries by
literal header). Specifically, in setStatus replace the direct interpolation of
id into the pattern used by new RegExp(`(## ${id}[\\s\\S]*?- \\*\\*Status:\\*\\*
)(...)`) by escaping regex metacharacters in id (or perform a literal string
search for the header `## ${id}` and then locate and replace the Status field),
then apply the same VALID_STATUSES check to perform the replacement safely.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 04802dcd-7243-4d8b-b920-cf588e979cf7
📒 Files selected for processing (5)
docs/SCRIPTS.mdlearning-queue.mjsmodes/_shared.mdpackage.jsontest-all.mjs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
learning-queue.mjs (2)
81-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden markdown field serialization to prevent queue corruption.
Line 86, Line 89, and Line 94 serialize user-controlled values verbatim; embedded newlines or ``` fences can break block structure and make subsequent parse/update operations target the wrong entry. Validate or escape single-line fields and fence markers before writing.
Suggested minimal fix
+function assertSingleLine(value, flag) { + if (/[\r\n]/.test(value)) throw new Error(`${flag} must be a single line`); +} + +function assertSafeProposal(value) { + if (value.includes('```')) throw new Error('--proposal cannot contain ``` fence markers'); +} + export function addEntry({ source, target, feedback, proposal, queuePath = QUEUE_PATH, date = today() }) { if (!source || !target || !feedback || !proposal) { throw new Error('add requires --source, --target, --feedback, and --proposal'); } + assertSingleLine(source, '--source'); + assertSingleLine(feedback, '--feedback'); + assertSafeProposal(proposal); if (!VALID_TARGETS.has(target)) { throw new Error(`target must be one of: ${Array.from(VALID_TARGETS).join(', ')}`); }Also applies to: 100-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@learning-queue.mjs` around lines 81 - 97, The renderEntry and addEntry flow currently writes user-controlled fields (source, target, feedback, proposal) verbatim which allows newlines or ``` fences to corrupt the markdown queue; add validation helpers (e.g., assertSingleLine(field, name) to reject or strip newline-containing values for single-line fields and assertSafeProposal(proposal) to reject or escape triple-backtick fences) and call them from addEntry before any validation of VALID_TARGETS; update renderEntry to either escape/encode any remaining unsafe characters in single-line fields or ensure addEntry has already sanitized them so that entry.proposal cannot contain "```" and source/feedback/target/date are single-line safe. Ensure function names referenced above (renderEntry, addEntry, assertSingleLine, assertSafeProposal, VALID_TARGETS) are used so subsequent parsing/updating of the queue cannot be broken by embedded newlines or fence markers.
129-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle missing queue file gracefully in
setStatus.Line 129 throws raw
ENOENTwhen the queue file does not exist; return a domain error instead (for example,queue file not found: <path>), then fail cleanly.Suggested minimal fix
export function setStatus({ id, status, queuePath = QUEUE_PATH }) { if (!id || !VALID_STATUSES.has(status)) { throw new Error('set-status requires --id and --status pending|applied|rejected'); } if (!ENTRY_ID_PATTERN.test(id)) { throw new Error('id must match LQ-YYYYMMDD-NNN'); } + if (!existsSync(queuePath)) { + throw new Error(`queue file not found: ${queuePath}`); + } const content = readFileSync(queuePath, 'utf-8');As per coding guidelines, "
**/*.mjs: Check for command injection, path traversal, and SSRF. Ensure scripts handle missing data/ directories gracefully."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@learning-queue.mjs` at line 129, The readFileSync call in setStatus (const content = readFileSync(queuePath, 'utf-8')) can throw raw ENOENT; wrap the file read in a try/catch inside setStatus and when err.code === 'ENOENT' return or throw a domain error with a clear message like "queue file not found: <queuePath>" (using the queuePath variable), while rethrowing other unexpected errors unchanged; ensure callers of setStatus receive the domain error so the process can fail cleanly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@learning-queue.mjs`:
- Around line 81-97: The renderEntry and addEntry flow currently writes
user-controlled fields (source, target, feedback, proposal) verbatim which
allows newlines or ``` fences to corrupt the markdown queue; add validation
helpers (e.g., assertSingleLine(field, name) to reject or strip
newline-containing values for single-line fields and
assertSafeProposal(proposal) to reject or escape triple-backtick fences) and
call them from addEntry before any validation of VALID_TARGETS; update
renderEntry to either escape/encode any remaining unsafe characters in
single-line fields or ensure addEntry has already sanitized them so that
entry.proposal cannot contain "```" and source/feedback/target/date are
single-line safe. Ensure function names referenced above (renderEntry, addEntry,
assertSingleLine, assertSafeProposal, VALID_TARGETS) are used so subsequent
parsing/updating of the queue cannot be broken by embedded newlines or fence
markers.
- Line 129: The readFileSync call in setStatus (const content =
readFileSync(queuePath, 'utf-8')) can throw raw ENOENT; wrap the file read in a
try/catch inside setStatus and when err.code === 'ENOENT' return or throw a
domain error with a clear message like "queue file not found: <queuePath>"
(using the queuePath variable), while rethrowing other unexpected errors
unchanged; ensure callers of setStatus receive the domain error so the process
can fail cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2384286f-5594-4f1b-bbe3-ac163ee62cbd
📒 Files selected for processing (1)
learning-queue.mjs
|
Fixed the valid CodeRabbit/CodeQL feedback on the learning queue. Changes:
Validation: |
|
@luochen211 — I'm closing this one and four siblings (#891, #892, #893, #894) together, and you deserve the full reasoning because the code quality isn't the issue. We're adopting an explicit acceptance criterion for the core, and these five fall on the other side of it: Core takes what the candidate uses; tooling that serves the project's own artifacts lives outside the core. career-ops needs to stay legible as "an agent that gets you a job" — for the user who clones it tomorrow, every additional validator/linter/manifest in Specific to this PR: it also modifies What I'd genuinely welcome: these five as a community companion repo (career-ops-devtools or similar — your repo, your pace, linked from our docs). The parity checker in particular (#893) may graduate into our internal CI if regional modes multiply. You went 4-for-4 on the merged half of this batch today (#886, #887, #888, #889 — all candidate-facing, all in), so the criterion clearly isn't about your work. It's about what the core is. 🙏 |
3 participants
Summary
learning-queue.mjsto record, list, and update pending profile-learning items indata/learning-queue.md.npm run learning, script docs, and shared mode guidance so feedback is queued before User Layer edits unless the user explicitly requests an immediate change.test-all.mjs.Fixes #882
Tests
node --check learning-queue.mjs && node learning-queue.mjs --self-testadd+list --status pendingwithCAREER_OPS_LEARNING_QUEUEtemporary filenode test-all.mjs --quick— 167 passed, 0 failed, 7 existing README.ua personal-data warningsSummary by CodeRabbit
New Features
Documentation
Tests