Skip to content

fix: raw SQL from db export -o {data} envelope; warn when create silently relinks directory - #211

Open
agent-zhang-beihai[bot] wants to merge 2 commits into
mainfrom
feedback/4b503324
Open

fix: raw SQL from db export -o {data} envelope; warn when create silently relinks directory#211
agent-zhang-beihai[bot] wants to merge 2 commits into
mainfrom
feedback/4b503324

Conversation

@agent-zhang-beihai

@agent-zhang-beihai agent-zhang-beihai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What was broken

Two frictions reported in the same feedback:

  1. db export --format sql -o file.sql wrote a JSON envelope. The backend can wrap the export payload as {format, data, tables}, but src/commands/db/export.ts only unwrapped the {format, content, tables} shape — any data-keyed response fell through to the raw-text branch and the .sql file ended up containing the JSON envelope instead of usable SQL.

  2. insforge create silently relinked the working directory. Creating a blank project overwrites .insforge/project.json in cwd with no notice when the directory was already linked to a different project — subsequent db query/db export commands then hit the wrong project unnoticed (the reporter's prod verification queries silently ran against staging).

What changed

  • src/commands/db/export.ts: new exported extractExportContent() accepts a string content or data key when unwrapping the export envelope, so -o file.sql writes raw SQL/JSON. Non-envelope responses still pass through verbatim.
  • src/commands/create.ts: new exported detectRelink() checks the existing project link before saveProjectConfig overwrites it. On a relink, interactive mode prints a prominent LINK CHANGED: this directory was linked to "<old>" ... and now points to "<new>" warning via clack.log.warn, and --json output gains a linkChanged: {previousProjectId, previousProjectName} field. Template projects link inside a freshly created subdirectory, so they're unaffected (correctly no warning).

No flags, defaults, or documented output shapes changed, so no agent-skills update is needed — the fix makes -o behave as the skill already documents.

Verification

  • New src/commands/db/export.test.ts: drives the real db export command with a mocked ossFetch{format, data} envelope → raw SQL written; {format, content} envelope still unwrapped; non-envelope response passes through verbatim; plus unit cases for extractExportContent.
  • New detectRelink cases in src/commands/create.test.ts: different project → previous link returned; unlinked dir → null; same project → null.
  • Full suite: npx vitest run → 646 passed / 13 skipped. npm run lint → 0 errors (1 pre-existing warning in unrelated apify.test.ts).

Addresses user feedback 4b503324-b3a5-4479-9b0d-7616c97a6dd4 (cli): CORRECTION: db export works — real issue is silently relinking + JSON envelope in -o output

🤖 Generated with Claude Code


Summary by cubic

Fixes db export to unwrap {content|data} envelopes so -o writes raw SQL/JSON and ensures --json includes the exported content. Also warns when create would relink the current directory to a different project and adds linkChanged to --json; addresses Linear feedback 4b503324.

  • Bug Fixes
    • db export: unwraps {format, content|data, tables}; non-envelope responses pass through; -o now writes raw SQL/JSON.
    • db export: --json now preserves content by returning { format, tables, content }.
    • create: detects relink before saving and warns in interactive mode; --json includes linkChanged with previous project info.

Written for commit 241d7eb. Summary will update on new commits.

Review in cubic

Note

Fix db export to unwrap data envelope and warn on silent relink in create

  • db export now correctly handles backend responses that wrap the SQL payload in either a content or data JSON field via the new extractExportContent helper in export.ts; previously only content was handled, so -o would write the raw JSON wrapper instead of SQL.
  • create now detects when a directory was previously linked to a different project via the new detectRelink function in create.ts, logging a warning in interactive mode or including a linkChanged object in JSON output.
  • Behavioral Change: db export -o with a data-enveloped response now writes raw SQL instead of the JSON envelope.

Macroscope summarized 47c6f1c.

…te relinks the directory

- db export: the backend can wrap exports in a {format, data} JSON
  envelope; only {format, content} was unwrapped, so '-o file.sql'
  wrote the envelope instead of raw SQL. Accept a string 'data' key
  too (extractExportContent) so the written file is directly usable.
- create: linking a blank project in cwd silently overwrote an
  existing .insforge/project.json pointing at a different project,
  so later db query/export commands hit the wrong project unnoticed.
  Detect the relink (detectRelink) and print a prominent LINK CHANGED
  warning; --json output gains a linkChanged field.
- Tests: new export.test.ts (envelope data/content unwrap + raw
  passthrough via the real command), detectRelink cases in
  create.test.ts.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@agent-zhang-beihai
agent-zhang-beihai Bot marked this pull request as ready for review July 27, 2026 19:19

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — fix: raw SQL from db export -o {data} envelope; warn on silent relink

Summary: Two focused, well-tested bug fixes — unwrapping the {data} export envelope and warning on a silent create relink; both implementations are correct and in-scope, with no blocking issues.

Requirements context

No matching spec/plan found under docs/specs/ (the only specs present are 2026-03-27-diagnose-* and 2026-04-17-db-migrations-command-design.md; there is no docs/superpowers/). Assessed against the PR description and the referenced Linear feedback 4b503324 alone.

Findings

Critical

(none) — both reported frictions are addressed and neither change alters existing flags, defaults, or output shapes.

Suggestion

  • Functionality — JSON-format exports may still write the envelope (src/commands/db/export.ts:14-18). extractExportContent only unwraps string content/data. The PR description claims "-o now writes raw SQL/JSON", but if the backend returns a JSON export as { format: 'json', data: { … } } (object-valued data), extractExportContent returns null and the -o file gets the full envelope again — i.e. the JSON half of the reported bug would be unfixed. Your own test returns null for non-string content/data pins this behavior for { format: 'json', data: { tables: [] } }, and there is no --format json -o end-to-end test. Worth confirming the backend's actual json-export response shape; if data is an object there, this case needs handling (e.g. JSON.stringify the object payload). Note this limitation also existed for the pre-existing content branch, so it is not a regression — just an unclosed gap relative to the PR's stated scope.

Information

  • Software engineering — --json output path is unchanged and untested (src/commands/db/export.ts:73-77). In --json mode the command emits outputJson(meta ?? { content }), i.e. only { format, tables } for an envelope response — the actual SQL/data is dropped — and --json combined with -o returns early at line 76 without writing the file. This is pre-existing behavior, not introduced here, and out of this PR's scope; flagging only because it is adjacent to the changed code and a one-line regression test would pin it.
  • Good structure. Extracting detectRelink (src/commands/create.ts:139-149) and extractExportContent into exported pure functions with focused unit tests matches the file's existing "logic extracted into pure functions" convention. The template-vs-blank distinction is correctly load-bearing on the process.chdir(projectDir) at src/commands/create.ts:353 — for templates detectRelink runs against the freshly-created empty subdir (→ no warning), for blank projects against the real cwd link (→ warning). The --json linkChanged field (create.ts:538-543) exposes only the user's own previous project id/name; no secrets/PII leaked, and api_key is not logged.
  • Test convention nit (src/commands/db/export.test.ts:44-48). Returning () => rmSync(dir, …) from beforeEach as teardown is valid vitest, but this pattern isn't used elsewhere in the suite; an explicit afterEach would read more consistently. Harmless as-is.

Security

No security-relevant regressions: no new user input reaches SQL/shell/HTTP unparameterized (export body is built from existing opts and posted as before), no secrets/tokens newly logged or returned, and no auth/authz checks touched (requireAuth still gates both commands).

Performance

No concerns: both additions are O(1) — a single extra getProjectConfig() file read on the create path and a two-key type check on export. No new loops, queries, or blocking I/O.

Verdict

approved (informational — the human still approves via the approve flow). Zero Critical findings; the two Suggestions/Information notes are non-blocking. The core fixes are correct, minimally scoped, and backed by tests that exercise the real command paths.

jwfing
jwfing previously approved these changes Jul 27, 2026

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - approved.

@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR improves database export response handling and reports when project creation changes the current directory’s link.

  • Unwraps string payloads from either content- or data-keyed database export responses.
  • Detects an existing link before saving a newly created project and reports the previous project in interactive and JSON output.
  • Adds unit and command-level coverage for both behaviors.

Confidence Score: 3/5

The PR is not yet safe to merge because malformed link files can orphan newly created projects and valid raw JSON exports can still be silently truncated.

The relink check reads and parses the old configuration only after creating the remote project, with no recovery for malformed JSON, while export envelope detection still treats ordinary string-valued data or content fields as wrappers and discards the rest of the document.

Files Needing Attention: src/commands/create.ts, src/commands/db/export.ts

Important Files Changed

Filename Overview
src/commands/create.ts Adds relink detection and reporting, but malformed existing project configuration can still abort after remote project creation.
src/commands/db/export.ts Adds support for data-keyed envelopes, but envelope detection still truncates matching raw JSON documents.
src/commands/create.test.ts Adds focused unit coverage for relink comparison but does not exercise malformed on-disk configuration.
src/commands/db/export.test.ts Covers supported wrappers and plain-text fallback but not raw JSON containing string-valued data or content fields.

Reviews (2): Last reviewed commit: "fix(db): preserve export content in JSON..." | Re-trigger Greptile

Comment thread src/commands/create.ts
// pointing at another project is about to be silently overwritten —
// detect it BEFORE saving so we can warn loudly (otherwise later
// db query/export commands hit the wrong project unnoticed).
const previousLink = detectRelink(getProjectConfig(), project.id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Malformed link aborts creation

When an existing .insforge/project.json contains malformed JSON, the new getProjectConfig() call throws after the remote project has been created but before saveProjectConfig() replaces the file, causing the command to exit with the remote project left unlinked from the working directory.

Knowledge Base Used: Project Lifecycle: Create, Link, List, and Branch

Comment thread src/commands/db/export.ts
Comment on lines +15 to +16
if (typeof parsed.content === 'string') return parsed.content;
if (typeof parsed.data === 'string') return parsed.data;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Envelope detection matches raw JSON

A raw JSON export with a top-level string-valued data or content property is treated as an envelope, so the output retains only that property instead of preserving the complete JSON document; require envelope-specific metadata such as format before unwrapping it.

Knowledge Base Used: Database commands (insforge db ...)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — #211 fix: raw SQL from db export -o {data} envelope; warn when create silently relinks directory

Summary: Tight, well-scoped two-part bugfix (export envelope unwrap + silent-relink warning) with good extracted-function unit tests; no blocking issues found.

Requirements context

No matching spec/plan found under docs/specs/ — the only specs present are 2026-03-27-diagnose-* and 2026-04-17-db-migrations-command-design.md, neither of which covers db export or create relink. Assessed against the PR description and linked feedback (4b503324) alone. This repo does not use docs/superpowers/.

Findings

Critical

(none)

Suggestion

  • [software engineering / test coverage] src/commands/create.ts:373 — the relink wiring is untested. detectRelink() has good pure-function tests (create.test.ts:154-171), but nothing verifies the two things that actually make this fix work: that getProjectConfig() is read before saveProjectConfig() overwrites it, and that the warning / linkChanged field fires. The original bug class is precisely "detect after save" — if a future refactor moved line 373 below line 383, previous.project_id would already equal project.id and detectRelink would silently return null, with all current tests still green. export.test.ts shows the command-level pattern (mock ossFetch/requireAuth, drive parseAsync); an analogous integration test asserting the linkChanged JSON on a pre-linked dir would lock in the regression guard.

  • [functionality] src/commands/db/export.ts:14-18extractExportContent only unwraps when content/data is a string. For --format json, if the backend wraps the payload as a JSON object ({format, data: {...}}), it returns null and -o dump.json writes the whole {format, data, tables} envelope rather than just the data — the same envelope-in-file symptom this PR fixes for SQL. export.test.ts:54 shows this is a deliberate, known limitation, and the reported issue was SQL, so blast radius is low. Worth confirming whether json-format exports need the same unwrap, or documenting that they intentionally pass through.

Information

  • src/commands/db/export.ts:65parsed.tables as string[] (and parsed.format as string) are unchecked casts; a malformed envelope would propagate a non-array into meta.tables. Only consumed for the .length count suffix (:81), so effectively harmless, but the cast hides the assumption.
  • src/commands/db/export.ts:73 — the --json output shape gains a content field vs. the previous outputJson(meta ?? {content}) which dropped content when an envelope was present. This is the intended fix (and matches the cubic summary), just noting the observable shape change for any downstream --json consumer.

Dimensions

  • Software engineering: Extracted pure functions + focused tests follow repo conventions; .js import extensions and type imports are consistent with the codebase. One integration-coverage gap noted above.
  • Functionality: Export unwrap and --json content-preservation are correct; verified getProjectConfig() reads cwd only (no upward tree-walk in config.ts:111-126), so the template-project path (which chdirs into a fresh empty subdir before linking) correctly produces no false warning, matching the PR claim.
  • Security: No security-relevant changes — no new user input reaches SQL/shell; the warning only echoes the user's own project id/name; no secrets logged. writeFileSync still targets the user-supplied -o path (unchanged, user-owned).
  • Performance: No concerns — trivial synchronous string/JSON work, no new I/O in a hot path.

Verdict

approved — zero Critical findings. Two Suggestions and two Information notes for the author's discretion; none blocking. (Human approval via the GitHub approve flow is still required.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants