feat: Add Integration System for Artifacts and Tasks - #300
feat: Add Integration System for Artifacts and Tasks#300HuyNguyenDinh wants to merge 13 commits into
Conversation
📝 WalkthroughWalkthroughAdds a configuration loader, a provider-agnostic task manager with Jira/Notion/Local adapters, a Notion sync utility and local map cache with CLI, numerous skill/docs updates to route saves through integrations, and tests covering config, adapters, and Notion sync behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI / sync command"
participant Config as "loadConfig()"
participant FS as "Local FS (.superpowers map)"
participant Notion as "Notion API"
CLI->>Config: loadConfig()
Config-->>CLI: config (NOTION key, roots)
CLI->>FS: getMap() / read map file
FS-->>CLI: map (may be {})
CLI->>FS: read local file(s) / derive title
CLI->>Notion: notionRequest(search/create page) with parentId
Notion-->>CLI: page info (id) or created page
CLI->>FS: saveMap(updated mapping)
FS-->>CLI: saved
CLI->>CLI: update/skip page content (append-only strategy)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@skills/test-driven-development/testing-anti-patterns.md`:
- Around line 292-314: The fenced code block under the "Gate Function" heading
is missing a language identifier (markdownlint MD040); update the opening fence
for that block (the triple backticks directly following the "Gate Function"
section) to include a language tag such as "text" (e.g., change ``` to ```text)
so the block is properly annotated.
In `@skills/verification-before-completion/SKILL.md`:
- Around line 78-86: Add a language tag to the fenced code block that begins
with "BEFORE claiming works:" in SKILL.md to satisfy markdownlint MD040; update
the opening ``` to ```text so the block reads ```text ... ``` and leave the
contents unchanged, ensuring the fenced block is properly closed with ``` at the
end.
🧹 Nitpick comments (7)
lib/config-core.js (2)
12-36: Consider potential issues with synchronous I/O and error details.A few observations:
TOCTOU race:
existsSyncfollowed byreadFileSynchas a small window where the file could be deleted. Consider wrapping the read in a try-catch and handlingENOENTspecifically.Swallowed error context: The warning message doesn't include the actual error, making debugging harder when JSON parsing fails.
Synchronous I/O: This is acceptable for CLI/startup scenarios but could block if called on a request path.
♻️ Optional: Include error message for easier debugging
try { userConfig = JSON.parse(fs.readFileSync(userConfigPath, 'utf8')); } catch (error) { - console.warn(`Warning: Failed to parse user config at ${userConfigPath}`); + console.warn(`Warning: Failed to parse user config at ${userConfigPath}: ${error.message}`); }try { projectConfig = JSON.parse(fs.readFileSync(projectConfigPath, 'utf8')); } catch (error) { - console.warn(`Warning: Failed to parse project config at ${projectConfigPath}`); + console.warn(`Warning: Failed to parse project config at ${projectConfigPath}: ${error.message}`); }
44-67: Verify prototype pollution safety and array merge behavior.Two considerations for
mergeDeep:
Prototype pollution: If a malicious config contains
__proto__orconstructorkeys, they could be assigned to the output object. Consider adding a check to skip dangerous keys.Array handling: Arrays are replaced entirely (source wins) rather than merged. This is a valid design choice but should be documented or tested to confirm intent.
🔒 Recommended: Guard against prototype pollution
function mergeDeep(target, source) { if (isObject(target) && isObject(source)) { const output = Object.assign({}, target); Object.keys(source).forEach(key => { + // Skip prototype pollution vectors + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { + return; + } if (isObject(source[key])) {skills/using-jira/SKILL.md (2)
27-46: Consider security note about credential handling in curl example.The curl example correctly uses environment variables for credentials. A few suggestions:
The
-D-flag dumps response headers to stdout, which may clutter output. Consider using-s(silent) or-wfor specific output formatting if headers aren't needed.Consider adding a note that
JIRA_API_TOKENshould be an API token (not password) for Atlassian Cloud, as basic auth with passwords is deprecated.📝 Optional: Simplify curl flags
```bash -curl -D- \ +curl -s \ -u $JIRA_EMAIL:$JIRA_API_TOKEN \
48-55: Documentation is incomplete for Update Ticket action.The Update Ticket section references "Jira REST API documentation" but doesn't provide an example like the Create Ticket section does. Consider adding a basic curl example for consistency, or at minimum a direct link to the relevant API docs.
skills/using-integrations/SKILL.md (1)
19-26: Relative import path may cause confusion.The import
'./lib/config-core.js'assumes the script is run from the repository root. This could fail or confuse users if they execute from a different directory.Consider either:
- Using an absolute path or package import (if published)
- Adding a note about the required working directory
- Showing how to resolve the path dynamically
📝 Optional: Add working directory note
### 1. Determine Configuration -First, run this Node.js script to check configuration: +First, run this Node.js script from the repository root to check configuration: ```javascripttests/lib/config-core.test.js (2)
27-73: Add test for malformed JSON handling.The test suite covers the happy paths well but doesn't test the error handling path where config files contain invalid JSON. This is important since
loadConfighas specific behavior (warn and continue with empty config) for parse failures.🧪 Suggested: Add test for malformed JSON
// Test 5: Malformed JSON should warn and return empty/partial config console.log('Test 5: Malformed JSON in user config'); fs.writeFileSync(path.join(USER_CONFIG_DIR, 'config.json'), '{ invalid json }'); fs.unlinkSync(path.join(PROJECT_CONFIG_DIR, 'config.json')); // Remove project config config = loadConfig(PROJECT_DIR); assert.deepStrictEqual(config, {}); // Should fall back to empty
1-1: Remove empty leading line.The file starts with an empty line before the imports. This is a minor style inconsistency.
✨ Suggested fix
- import { loadConfig } from '../../lib/config-core.js';
|
Can you give me a human-written description of what you were trying to do with this PR? |
I'm trying to integrate Notion for storing Documents and Implementation Plans which storing in Local or Git (if you commit), beside that I've realized that we didn't applied the project management concept to improve QA/QC stage so I'm trying to to add the jira or other management tools for work logs which make us visible what the agents did, our performances with agentic workflow and where we stuck at most. |
- Add `lib/notion-sync.js` for recursive documentation mirroring - Add `lib/task-manager.js` with adapters for Notion, Jira, and Local - Update `lib/config-core.js` to support new configuration schemas - Update `skills/using-notion` to use new sync script - Add `skills/using-task-tracker` and deprecate `skills/using-jira`
- Fix Notion adapter fallback to return actual block ID instead of hardcoded 'fallback' - Add error handling for malformed JSON in notion-sync getMap() - Add directory creation in notion-sync saveMap() to prevent ENOENT errors - Fix parent ID normalization to handle both hyphenated and non-hyphenated formats - Add project_key to Jira config defaults for completeness - Add comprehensive tests for all fixes using TDD All fixes implemented following Test-Driven Development: - tests/lib/notion-adapter.test.js: Test fallback block ID capture - tests/lib/notion-sync.test.js: Test file system safety and ID normalization Resolves CodeRabbit review comments from PR #3 Co-Authored-By: Claude <noreply@anthropic.com>
- Update documentation to clarify mapping store source of truth - Add proper validation and error handling to Jira adapter - Fix local adapter ID collision issue with better random generation - Add .superpowers/ to .gitignore - Fix trailing whitespace in notion-sync.js These changes address remaining review feedback and code quality improvements. Co-Authored-By: Claude <noreply@anthropic.com>
The test was expecting an empty object when JSON is malformed, but with the new default config system, it should return defaults. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docs/plans/2026-01-20-notion-integration-design.md`:
- Around line 30-32: The nested unordered list under the "Skill Instructions"
heading uses a 3-space indent instead of the 2-space indent markdownlint
expects; fix by adjusting the nested list lines (the bullets referencing "Update
`skills/using-notion/SKILL.md`..." and "Since skills are text instructions, we
might need a helper script...") to use exactly two spaces before the "-" so the
nested bullets are indented by 2 spaces relative to the parent list item; keep
the content unchanged aside from whitespace.
In `@lib/adapters/jira-adapter.js`:
- Around line 89-100: The createSubtask method builds a URL from
response.id/response.key without validating the response like createTask does;
update createSubtask to validate the response returned from
this.request('issue', 'POST', body) (e.g., ensure response && response.id &&
response.key), and if validation fails throw or return an explicit
error/nullable value instead of constructing a URL with undefined values;
reference the createSubtask function and mirror the validation logic/pattern
used in createTask to locate and implement the fix.
In `@lib/adapters/local-adapter.js`:
- Around line 27-35: In logWork, you build a RegExp directly from the
user-supplied id which can contain regex metacharacters; fix by escaping the id
before constructing the RegExp (e.g., add an escapeRegExp utility that replaces
[.*+?^${}()|[\]\\] with backslash-escaped versions) and use const taskRegex =
new RegExp(`## \\[${escapeRegExp(id)}\\].*`, 'g') (or import a safe escaping
helper) so the regex matches the literal id.
In `@lib/notion-sync.js`:
- Around line 165-181: updatePageContent currently returns early when
children.results.length > 0, silently skipping updates to non-empty pages;
change it to support an explicit opt-in overwrite mode and make the behavior
visible: add an overwrite boolean parameter to updatePageContent (and propagate
it from the CLI option), and when overwrite is true iterate over
children.results and remove/archive each block via notionRequest (use the same
"blocks/{id}" endpoint and appropriate delete/archive request), then append new
blocks; when overwrite is false keep the current safe behavior but replace the
console.log with a clear warning that includes the pageId and guidance to use
--overwrite or README for full sync; update the CLI help/README to document the
--overwrite flag and its consequences.
- Around line 99-113: The page creation payload uses an incorrect shape for the
title property when creating a page with parent: { page_id: ... } so the Notion
API rejects it; update the payload built in the code that calls notionRequest
(the newPage creation block) so that properties.title is an object with a title
array (i.e., properties: { title: { title: [ ... ] } } ) and each entry uses the
Notion rich text shape (type: 'text', text: { content: title }) instead of the
current direct array; keep children as markdownToBlocks(content) and keep
parent: { page_id: parentId } unchanged.
In `@lib/task-manager.js`:
- Around line 1-4: Replace any use of import.meta.filename with a cross-version
approach: import fileURLToPath from 'url' and path from 'path', compute const
__filename = fileURLToPath(import.meta.url) and const __dirname =
path.dirname(path.resolve(__filename)), then use path.resolve(__dirname, ...)
for comparisons; update this in lib/task-manager.js (where import.meta.filename
is used) and lib/notion-sync.js (around line 26) and ensure all path comparisons
use path.resolve for reliable behavior across Node versions.
In `@tests/lib/config-core.test.js`:
- Around line 77-82: In the catch block that currently calls process.exit(1),
replace that immediate exit with setting process.exitCode = 1 so the process
will signal failure without aborting execution; leave the console.error call
intact and allow the finally block to call teardown() to restore os.homedir and
remove TEST_DIR (i.e., change the usage of process.exit(1) to process.exitCode =
1 in the catch surrounding teardown()).
🧹 Nitpick comments (3)
tests/lib/notion-sync.test.js (1)
9-90: Avoid duplicating production helpers in tests.These tests reimplement
getMap,saveMap, andsearchChildPage. If the production logic changes, tests may still pass. Consider exporting these helpers fromlib/notion-sync.js(or adding a MAP_FILE override) so tests exercise the real implementation.lib/notion-sync.js (1)
28-53: Add a request timeout to Notion API calls.Without a timeout, a hung network call can stall the CLI indefinitely. The Jira adapter implements this pattern with a 30-second timeout using AbortController; apply the same approach to
notionRequest.🕒 Suggested timeout addition
export async function notionRequest(endpoint, method = 'GET', body = null) { if (!NOTION_API_KEY) { throw new Error("Missing Notion API Key in config"); } + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + const headers = { 'Authorization': `Bearer ${NOTION_API_KEY}`, 'Notion-Version': NOTION_VERSION, 'Content-Type': 'application/json' }; const options = { method, - headers + headers, + signal: controller.signal }; if (body) { options.body = JSON.stringify(body); } - const response = await fetch(`https://api.notion.com/v1/${endpoint}`, options); - if (!response.ok) { - const text = await response.text(); - throw new Error(`Notion API Error ${response.status}: ${text}`); - } - return response.json(); + try { + const response = await fetch(`https://api.notion.com/v1/${endpoint}`, options); + if (!response.ok) { + const text = await response.text(); + throw new Error(`Notion API Error ${response.status}: ${text}`); + } + return response.json(); + } finally { + clearTimeout(timeoutId); + } }lib/adapters/notion-adapter.js (1)
11-90: Validate database schema on initialization to fail fast on property mismatches.The
createTaskandcreateSubtaskmethods hardcode property names (Name,Status,Type,Parent Item) without verifying they exist in the Notion database. If the schema differs, calls fail with opaque API errors. Use the Notion API'sGET /v1/databases/{database_id}endpoint in the constructor to validate that required properties exist and have correct types, so configuration errors surface immediately rather than at runtime.
| - **Skill Instructions**: | ||
| - Update `skills/using-notion/SKILL.md` to include a recursive strategy or a script reference. | ||
| - Since skills are text instructions, we might need a helper script in `lib/notion-sync.js` to handle the recursion complexity, which the skill then invokes. |
There was a problem hiding this comment.
Fix unordered list indentation (MD007).
markdownlint expects a 2‑space indent for nested list items here.
🧹 Proposed fix
- - Update `skills/using-notion/SKILL.md` to include a recursive strategy or a script reference.
- - Since skills are text instructions, we might need a helper script in `lib/notion-sync.js` to handle the recursion complexity, which the skill then invokes.
+ - Update `skills/using-notion/SKILL.md` to include a recursive strategy or a script reference.
+ - Since skills are text instructions, we might need a helper script in `lib/notion-sync.js` to handle the recursion complexity, which the skill then invokes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Skill Instructions**: | |
| - Update `skills/using-notion/SKILL.md` to include a recursive strategy or a script reference. | |
| - Since skills are text instructions, we might need a helper script in `lib/notion-sync.js` to handle the recursion complexity, which the skill then invokes. | |
| - **Skill Instructions**: | |
| - Update `skills/using-notion/SKILL.md` to include a recursive strategy or a script reference. | |
| - Since skills are text instructions, we might need a helper script in `lib/notion-sync.js` to handle the recursion complexity, which the skill then invokes. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
31-31: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
🤖 Prompt for AI Agents
In `@docs/plans/2026-01-20-notion-integration-design.md` around lines 30 - 32, The
nested unordered list under the "Skill Instructions" heading uses a 3-space
indent instead of the 2-space indent markdownlint expects; fix by adjusting the
nested list lines (the bullets referencing "Update
`skills/using-notion/SKILL.md`..." and "Since skills are text instructions, we
might need a helper script...") to use exactly two spaces before the "-" so the
nested bullets are indented by 2 spaces relative to the parent list item; keep
the content unchanged aside from whitespace.
| async createSubtask(parentId, title) { | ||
| const body = { | ||
| fields: { | ||
| project: { key: this.projectKey }, | ||
| parent: { key: parentId }, | ||
| summary: title, | ||
| issuetype: { name: 'Sub-task' } | ||
| } | ||
| }; | ||
| const response = await this.request('issue', 'POST', body); | ||
| return { id: response.id, key: response.key, url: `${this.host}/browse/${response.key}` }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file size and read the jira-adapter.js file
wc -l lib/adapters/jira-adapter.jsRepository: obra/superpowers
Length of output: 176
🏁 Script executed:
# Read the full file to understand the context and find createTask method
cat -n lib/adapters/jira-adapter.jsRepository: obra/superpowers
Length of output: 4242
🌐 Web query:
Jira REST API 3 create issue response format id key
💡 Result:
When you successfully create an issue with Jira REST API v3 (POST /rest/api/3/issue) the response body is JSON containing at least:
- id (string) — the internal numeric ID as a string
- key (string) — the issue key (e.g. PROJ-123)
- self (string) — URL to the created issue
Example:
{"id":"15735","key":"SSP-28","self":"https://your-site.atlassian.net/rest/api/3/issue/15735"}. Bulk-create returns an object with an "issues" array of those entries. [1][2]
Sources:
[1] Jira Cloud REST API — Create issue (response schema).
[2] Jira REST API example create issue (example response).
Validate sub-task creation response before building the URL.
createSubtask lacks response validation that createTask already has (line 52). If Jira returns an unexpected payload, this returns undefined id/key and a broken URL. Add the validation check:
async createSubtask(parentId, title) {
const body = {
fields: {
project: { key: this.projectKey },
parent: { key: parentId },
summary: title,
issuetype: { name: 'Sub-task' }
}
};
const response = await this.request('issue', 'POST', body);
+ if (!response?.id || !response?.key) {
+ throw new Error('Unexpected Jira API response: missing id or key');
+ }
return { id: response.id, key: response.key, url: `${this.host}/browse/${response.key}` };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async createSubtask(parentId, title) { | |
| const body = { | |
| fields: { | |
| project: { key: this.projectKey }, | |
| parent: { key: parentId }, | |
| summary: title, | |
| issuetype: { name: 'Sub-task' } | |
| } | |
| }; | |
| const response = await this.request('issue', 'POST', body); | |
| return { id: response.id, key: response.key, url: `${this.host}/browse/${response.key}` }; | |
| } | |
| async createSubtask(parentId, title) { | |
| const body = { | |
| fields: { | |
| project: { key: this.projectKey }, | |
| parent: { key: parentId }, | |
| summary: title, | |
| issuetype: { name: 'Sub-task' } | |
| } | |
| }; | |
| const response = await this.request('issue', 'POST', body); | |
| if (!response?.id || !response?.key) { | |
| throw new Error('Unexpected Jira API response: missing id or key'); | |
| } | |
| return { id: response.id, key: response.key, url: `${this.host}/browse/${response.key}` }; | |
| } |
🤖 Prompt for AI Agents
In `@lib/adapters/jira-adapter.js` around lines 89 - 100, The createSubtask method
builds a URL from response.id/response.key without validating the response like
createTask does; update createSubtask to validate the response returned from
this.request('issue', 'POST', body) (e.g., ensure response && response.id &&
response.key), and if validation fails throw or return an explicit
error/nullable value instead of constructing a URL with undefined values;
reference the createSubtask function and mirror the validation logic/pattern
used in createTask to locate and implement the fix.
| async logWork(id, timeSpent, comment = '') { | ||
| this.ensureFile(); | ||
| let content = fs.readFileSync(this.filePath, 'utf8'); | ||
| const taskRegex = new RegExp(`## \\[${id}\\].*`, 'g'); | ||
| const match = taskRegex.exec(content); | ||
|
|
||
| if (!match) { | ||
| throw new Error(`Task ${id} not found locally.`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and examine the relevant lines
cat -n lib/adapters/local-adapter.js | sed -n '20,45p'Repository: obra/superpowers
Length of output: 1461
🏁 Script executed:
# Check if there are any escaping utilities already defined in the file
rg -A 5 -B 5 "escape|regex" lib/adapters/local-adapter.jsRepository: obra/superpowers
Length of output: 699
🏁 Script executed:
# Search for where logWork is called to understand how id is provided
rg -n "logWork\(" --type jsRepository: obra/superpowers
Length of output: 386
🏁 Script executed:
# Look for any validation or sanitization of the id parameter
rg -B 10 "logWork.*id" lib/adapters/local-adapter.jsRepository: obra/superpowers
Length of output: 540
🏁 Script executed:
# Check the context around line 50 in task-manager.js where logWork is called
cat -n lib/task-manager.js | sed -n '40,65p'Repository: obra/superpowers
Length of output: 1378
🏁 Script executed:
# Look for where the id in task-manager.js comes from (user input, parsing, etc.)
cat -n lib/task-manager.js | head -70Repository: obra/superpowers
Length of output: 2739
Escape task IDs before building the regex.
The id parameter is user-supplied via CLI arguments and passed directly to the regex constructor without sanitization. If a user provides an id containing regex metacharacters (e.g., test[123] or task.*), the regex will behave unexpectedly or fail to match.
🔧 Proposed fix
- const taskRegex = new RegExp(`## \\[${id}\\].*`, 'g');
+ const escapedId = id.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ const taskRegex = new RegExp(`## \\[${escapedId}\\].*`);🤖 Prompt for AI Agents
In `@lib/adapters/local-adapter.js` around lines 27 - 35, In logWork, you build a
RegExp directly from the user-supplied id which can contain regex
metacharacters; fix by escaping the id before constructing the RegExp (e.g., add
an escapeRegExp utility that replaces [.*+?^${}()|[\]\\] with backslash-escaped
versions) and use const taskRegex = new RegExp(`## \\[${escapeRegExp(id)}\\].*`,
'g') (or import a safe escaping helper) so the regex matches the literal id.
| // 3. Create Page | ||
| console.log(`Creating new page for "${title}"...`); | ||
| const newPage = await notionRequest('pages', 'POST', { | ||
| parent: { page_id: parentId }, | ||
| properties: { | ||
| title: [ | ||
| { | ||
| text: { | ||
| content: title | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| children: content ? markdownToBlocks(content) : [] | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "notion-sync.js" -type f | head -5Repository: obra/superpowers
Length of output: 176
🏁 Script executed:
git ls-files | grep -E "(notion|test)" | head -20Repository: obra/superpowers
Length of output: 889
🌐 Web query:
Notion API create page with page_id parent properties title format
💡 Result:
Use the pages.create endpoint with parent.type "page_id" and set properties with a single "title" property whose value is a title-type array of rich-text objects.
Minimal JSON (curl or body for SDK):
{
"parent": { "type": "page_id", "page_id": "YOUR_PAGE_ID" },
"properties": {
"title": {
"type": "title",
"title": [
{
"type": "text",
"text": { "content": "My new page title", "link": null }
}
]
}
}
}
JS (notion-client) example:
await notion.pages.create({
parent: { page_id: "YOUR_PAGE_ID" },
properties: {
title: {
type: "title",
title: [{ type: "text", text: { content: "My new page title" } }]
}
}
});
Notes:
- When parent.type is "page_id", the only valid key in properties is the title property. (developers.notion.com)
Sources: Notion API reference — Create a page; Working with page content; Page property values. (developers.notion.com)
Citations:
- 1: https://developers.notion.com/reference/post-page?utm_source=openai
- 2: https://developers.notion.com/reference/post-page?utm_source=openai
🏁 Script executed:
sed -n '90,120p' lib/notion-sync.jsRepository: obra/superpowers
Length of output: 809
🏁 Script executed:
grep -n "notionRequest" lib/notion-sync.js | head -5Repository: obra/superpowers
Length of output: 406
🏁 Script executed:
rg "function notionRequest|const notionRequest|export.*notionRequest" -A 10Repository: obra/superpowers
Length of output: 608
🏁 Script executed:
sed -n '1,40p' lib/notion-sync.jsRepository: obra/superpowers
Length of output: 1216
🏁 Script executed:
sed -n '28,55p' lib/notion-sync.jsRepository: obra/superpowers
Length of output: 791
🏁 Script executed:
rg "title.*\[" lib/ --type js -B 2 -A 2Repository: obra/superpowers
Length of output: 1783
🏁 Script executed:
grep -r "title.*type.*title" . --include="*.js" --include="*.md"Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
cat lib/adapters/notion-adapter.js | head -80Repository: obra/superpowers
Length of output: 2543
🏁 Script executed:
find . -name "*.md" -type f -exec grep -l "notion\|Notion" {} \; | head -5Repository: obra/superpowers
Length of output: 272
🏁 Script executed:
cat ./docs/plans/2026-01-20-notion-integration-design.md | head -100Repository: obra/superpowers
Length of output: 2811
🏁 Script executed:
rg "parent.*page_id|parent.*database_id" lib/ -B 2 -A 5Repository: obra/superpowers
Length of output: 2592
Fix page creation payload for page-parented pages.
The Notion API rejects the current title property format when creating a page with parent: { page_id: ... }. The API requires title to be an object containing a title array, not a direct array.
Proposed fix
const newPage = await notionRequest('pages', 'POST', {
parent: { page_id: parentId },
properties: {
- title: [
- {
- text: {
- content: title
- }
- }
- ]
+ title: {
+ title: [
+ {
+ text: {
+ content: title
+ }
+ }
+ ]
+ }
},
children: content ? markdownToBlocks(content) : []
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 3. Create Page | |
| console.log(`Creating new page for "${title}"...`); | |
| const newPage = await notionRequest('pages', 'POST', { | |
| parent: { page_id: parentId }, | |
| properties: { | |
| title: [ | |
| { | |
| text: { | |
| content: title | |
| } | |
| } | |
| ] | |
| }, | |
| children: content ? markdownToBlocks(content) : [] | |
| }); | |
| // 3. Create Page | |
| console.log(`Creating new page for "${title}"...`); | |
| const newPage = await notionRequest('pages', 'POST', { | |
| parent: { page_id: parentId }, | |
| properties: { | |
| title: { | |
| title: [ | |
| { | |
| text: { | |
| content: title | |
| } | |
| } | |
| ] | |
| } | |
| }, | |
| children: content ? markdownToBlocks(content) : [] | |
| }); |
🤖 Prompt for AI Agents
In `@lib/notion-sync.js` around lines 99 - 113, The page creation payload uses an
incorrect shape for the title property when creating a page with parent: {
page_id: ... } so the Notion API rejects it; update the payload built in the
code that calls notionRequest (the newPage creation block) so that
properties.title is an object with a title array (i.e., properties: { title: {
title: [ ... ] } } ) and each entry uses the Notion rich text shape (type:
'text', text: { content: title }) instead of the current direct array; keep
children as markdownToBlocks(content) and keep parent: { page_id: parentId }
unchanged.
| async function updatePageContent(pageId, content) { | ||
| // Notion API replacement is complex (block management). | ||
| // For MVP, we might just append or replace if possible, but Notion API doesn't support "replace all content" easily. | ||
| // We would need to delete all blocks and append new ones. | ||
| console.log(`Updating content for page ${pageId}...`); | ||
|
|
||
| // 1. Get existing blocks | ||
| const children = await notionRequest(`blocks/${pageId}/children`, 'GET'); | ||
|
|
||
| // 2. Delete existing blocks (batching helps, but doing one by one for safety/simplicity first) | ||
| // Notion limits this. For a robust sync, this part needs care. | ||
| // For this implementation plan, we will just Append if empty, or warn if not. | ||
| if (children.results.length > 0) { | ||
| console.log(`Page ${pageId} is not empty. Skipping content overwrite for safety in MVP.`); | ||
| // In full version: Archive all children blocks, then append new ones. | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and size
wc -l lib/notion-sync.js
head -20 lib/notion-sync.jsRepository: obra/superpowers
Length of output: 706
🏁 Script executed:
# Look at the specific function around lines 165-181
sed -n '160,190p' lib/notion-sync.jsRepository: obra/superpowers
Length of output: 1376
🏁 Script executed:
# Search for other sync-related functions and patterns to understand intended behavior
rg -A 5 -B 5 'updatePageContent|syncStrategy|append|replace' lib/notion-sync.jsRepository: obra/superpowers
Length of output: 2344
🏁 Script executed:
# Look for README or documentation about MVP status
find . -maxdepth 2 -type f \( -name "README*" -o -name "DESIGN*" -o -name "*.md" \) | head -20Repository: obra/superpowers
Length of output: 337
🏁 Script executed:
# Search for TODO/FIXME comments related to sync or update behavior
rg -i 'todo|fixme|full.version|mvp' lib/notion-sync.jsRepository: obra/superpowers
Length of output: 434
🏁 Script executed:
# Check if there are any configuration options for sync behavior
rg -B 3 -A 3 'sync|update|append|overwrite' lib/config-core.js | head -50Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
# Look at how syncFile is exported and used
rg -B 5 -A 10 'export.*syncFile|syncFile' lib/notion-sync.js | head -100Repository: obra/superpowers
Length of output: 1784
Sync skips updates to non-empty pages by design, but this limitation should be more visible.
updatePageContent returns early for pages with existing blocks, preventing updates when syncing file changes. This is an intentional MVP limitation noted in the code ("safety"), but it creates a gap between what users expect from a sync tool and the actual behavior. File changes won't propagate to Notion without manually clearing the page first.
Consider one of:
- Implement full block deletion and replacement in MVP (removing the safety guard)
- Add a
--overwriteflag to allow users to opt into updates for non-empty pages - Document this limitation clearly in the CLI help and README to set expectations
🤖 Prompt for AI Agents
In `@lib/notion-sync.js` around lines 165 - 181, updatePageContent currently
returns early when children.results.length > 0, silently skipping updates to
non-empty pages; change it to support an explicit opt-in overwrite mode and make
the behavior visible: add an overwrite boolean parameter to updatePageContent
(and propagate it from the CLI option), and when overwrite is true iterate over
children.results and remove/archive each block via notionRequest (use the same
"blocks/{id}" endpoint and appropriate delete/archive request), then append new
blocks; when overwrite is false keep the current safe behavior but replace the
console.log with a clear warning that includes the pageId and guidance to use
--overwrite or README for full sync; update the CLI help/README to document the
--overwrite flag and its consequences.
| import { loadConfig } from './config-core.js'; | ||
| import { LocalAdapter } from './adapters/local-adapter.js'; | ||
| import { NotionAdapter } from './adapters/notion-adapter.js'; | ||
| import { JiraAdapter } from './adapters/jira-adapter.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file
find . -name "task-manager.js" -type fRepository: obra/superpowers
Length of output: 81
🏁 Script executed:
# Read the file to see the imports and the CLI check code
cat -n ./lib/task-manager.jsRepository: obra/superpowers
Length of output: 2739
🏁 Script executed:
# Let me verify Node.js ESM import.meta properties by checking Node.js docs and testing behavior
# First, check if there are any other Node.js files that might use import.meta to understand patterns
rg "import\.meta\." lib/ --type jsRepository: obra/superpowers
Length of output: 194
🏁 Script executed:
# Check the package.json for Node version requirements
cat package.json | head -30Repository: obra/superpowers
Length of output: 104
🌐 Web query:
Node.js ESM import.meta.filename vs import.meta.url fileURLToPath
💡 Result:
Short answer
- import.meta.url — built-in ESM meta property: the module's absolute file: URL (works in Node and browsers). Use it with url/file APIs. [1][2]
- fileURLToPath(import.meta.url) — Node utility to convert that file: URL to a filesystem path (same string format as CommonJS __filename). [1][2]
- import.meta.filename (and import.meta.dirname) — Node added these convenience properties (introduced v20.11.0/v21.2.0 as experimental, stabilized in later releases): they equal fileURLToPath(import.meta.url) (filename with symlinks resolved) and path.dirname(...) respectively. Only available for file: modules. [1][3]
Minimal examples
-
canonical conversion:
import { fileURLToPath } from 'url';
const __filename = fileURLToPath(import.meta.url);
const __dirname = new URL('.', import.meta.url).pathname; // or path.dirname(__filename) -
using import.meta.filename (Node >= v20.11 / stable later):
// in an ESM file
console.log(import.meta.filename); // absolute path (symlinks resolved)
console.log(import.meta.dirname); // directory
Notes
- import.meta.filename/dirname are Node-specific (not in browsers) and only for file: URLs. For maximum compatibility use fileURLToPath(import.meta.url). [1][2][3]
Sources
[1] Node.js ESM docs (import.meta.url, fileURLToPath examples).
[2] MDN — import.meta and URL-based resolution examples.
[3] Node.js docs (import.meta.filename / dirname added in v20.11.0/v21.2.0; stabilized in later releases).
import.meta.filename is Node-specific and only available in v20.11.0+
This won't work reliably across Node versions. Use fileURLToPath(import.meta.url) and path.resolve() for cross-version compatibility and proper path comparison.
🔧 Proposed fix
import { loadConfig } from './config-core.js';
import { LocalAdapter } from './adapters/local-adapter.js';
import { NotionAdapter } from './adapters/notion-adapter.js';
import { JiraAdapter } from './adapters/jira-adapter.js';
+import path from 'path';
+import { fileURLToPath } from 'url';
const config = loadConfig();
const pmConfig = config.project_management || {};
@@ -23,7 +25,8 @@ switch (provider) {
}
// CLI Logic
-if (process.argv[1] === import.meta.filename) {
+const entryFile = process.argv[1] ? path.resolve(process.argv[1]) : null;
+if (entryFile === fileURLToPath(import.meta.url)) {Also applies to: lib/notion-sync.js:26
🤖 Prompt for AI Agents
In `@lib/task-manager.js` around lines 1 - 4, Replace any use of
import.meta.filename with a cross-version approach: import fileURLToPath from
'url' and path from 'path', compute const __filename =
fileURLToPath(import.meta.url) and const __dirname =
path.dirname(path.resolve(__filename)), then use path.resolve(__dirname, ...)
for comparisons; update this in lib/task-manager.js (where import.meta.filename
is used) and lib/notion-sync.js (around line 26) and ensure all path comparisons
use path.resolve for reliable behavior across Node versions.
| } catch (err) { | ||
| console.error('Test failed:', err); | ||
| process.exit(1); | ||
| } finally { | ||
| teardown(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the test file
fd "config-core.test.js" tests/Repository: obra/superpowers
Length of output: 89
🏁 Script executed:
# Read the specific lines with context
if [ -f "tests/lib/config-core.test.js" ]; then
cat -n "tests/lib/config-core.test.js" | sed -n '60,95p'
fiRepository: obra/superpowers
Length of output: 1275
🏁 Script executed:
# Search for teardown definition in the file
if [ -f "tests/lib/config-core.test.js" ]; then
rg "teardown" "tests/lib/config-core.test.js" -B 3 -A 3
fiRepository: obra/superpowers
Length of output: 355
🌐 Web query:
Node.js process.exit() finally block execution behavior
💡 Result:
Short answer: calling process.exit() can prevent a try...finally finally block from running. Node's process.exit() instructs the runtime to terminate synchronously "as quickly as possible" (it only guarantees that 'exit' listeners run synchronously), so if you call it before the finally is reached the finally code may never execute. Use process.exitCode or allow the event loop to drain for a graceful shutdown instead. [1][2]
Sources:
- Node.js process.exit() docs — notes synchronous termination and that only 'exit' listeners are run. [1]
- Discussion/answers noting process.exit() as a cause for finally not executing. [2]
Replace process.exit(1) with process.exitCode = 1 to ensure teardown runs.
The process.exit() call bypasses the finally block, preventing teardown() from restoring os.homedir and removing the temp TEST_DIR. This leaves the test environment in a dirty state when tests fail.
Suggested fix
} catch (err) {
console.error('Test failed:', err);
- process.exit(1);
+ process.exitCode = 1;
} finally {
teardown();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| console.error('Test failed:', err); | |
| process.exit(1); | |
| } finally { | |
| teardown(); | |
| } | |
| } catch (err) { | |
| console.error('Test failed:', err); | |
| process.exitCode = 1; | |
| } finally { | |
| teardown(); | |
| } |
🤖 Prompt for AI Agents
In `@tests/lib/config-core.test.js` around lines 77 - 82, In the catch block that
currently calls process.exit(1), replace that immediate exit with setting
process.exitCode = 1 so the process will signal failure without aborting
execution; leave the console.error call intact and allow the finally block to
call teardown() to restore os.homedir and remove TEST_DIR (i.e., change the
usage of process.exit(1) to process.exitCode = 1 in the catch surrounding
teardown()).
Adds missing unit and integration tests for: - LocalAdapter (file creation, task appending) - JiraAdapter (payload validation, mocking) - TaskManager (CLI argument integration)
|
Hi. These changes don't look like a great fit for Superpowers writ large. I'm not particularly interested in adding direct Notion and Jira integration. |
2 participants
Summary
lib/config-core.jsfor loading project/user configuration.using-integrationsskill as a router for storage and task management.using-notionandusing-jiraskills for external integrations.writing-plansandbrainstormingskills to use the new integration system.Test Plan
config-core.jspass.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.