Skip to content

lineage generation - #65

Merged
joroscoSF merged 8 commits into
mainfrom
jo_gen_lineage
Oct 16, 2026
Merged

joroscoSF merged 8 commits into
mainfrom
jo_gen_lineage

Conversation

@joroscoSF

@joroscoSF joroscoSF commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

adds hidden (fow now) command

sf data-code-extension script lineage generate --help

Screenshot 2026-10-15 at 8 58 02 AM

…ing in tests

- scan: re-read config.json after confirmation and refuse to write if it
  changed while the prompt was pending (ScanConfigChangedDuringConfirm)
- lineage generate: treat root-level requirements.txt,
  build_native_dependencies.sh and Dockerfile.dependencies as package
  inputs in the --output-file collision check
- zipBuilder tests: strip only the trailing :/workspace from the docker
  mount so Windows drive-letter paths parse correctly
- compare the output path against input files by resolved path as well as
  file identity, so build files that don't exist yet are still protected
- follow dangling symlinks to the path a write would land on
- compare case-insensitively on macOS and Windows
- scan: a declined stale-lineage prompt no longer reports the scan as
  applied; it returns success: false and prints only the not-written notice
- scan: always re-check config.json before writing, not only after a prompt
- lineage generate: abort the write if any part of config.json (including
  permissions the gaps were checked against) changed during confirmation
- lineage generate: fail fast when --output-file's directory isn't writable
  and when the target org has no username, instead of after the run or
  authenticating as a placeholder org
- diffLineage: compare target and source fields case-insensitively, as
  deploy validation does
- validate the SDK result shape (LineageResultInvalid) instead of casting

@jcatt-sf jcatt-sf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review (Claude Code /code-review) of 11282a3. 15 findings, each posted inline. Here they are, most severe first:

  1. src/utils/pythonRunner.ts:210: Timeout kills only the shim; a null exit code is treated as success
  2. src/base/lineageBase.ts:265: Write refusal under --json/non-TTY happens only after the full run
  3. src/base/lineageBase.ts:115: --output-file target isn't validated before the run
  4. src/base/lineageBase.ts:182: Empty trace leaves a stale dataLineage in place and reports success
  5. src/base/lineageBase.ts:227: Replace prompt shows only counts, not the affected entries
  6. messages/lineage.md:7: Help says nothing is written, but external calls still run
  7. src/base/lineageBase.ts:248: Permission-gap remedy ('run scan') can't fix dynamically built names
  8. src/base/lineageBase.ts:90: Streaming scripts aren't rejected before the run
  9. src/base/lineageBase.ts:280: Prompt key uses replacing instead of needsReplace
  10. src/base/scanBase.ts:142: scan now fails non-interactively when an existing dataLineage goes stale
  11. src/base/lineageBase.ts:212: No writability pre-check for config.json
  12. messages/lineage.md:222: --dependencies help misdescribes what the flag does
  13. src/utils/lineageConfig.ts:157: config.json trailing newline differs from scan/init
  14. src/utils/lineageRunner.ts:180: SDK lineage entries are cast without shape validation
  15. src/utils/lineageRunner.ts:159: Temp-dir cleanup failure can mask the run result

The code ?? 0 line behind item 1 predates this PR, but lineage generate now runs through it, so that comment is attached to L212, the nearest line in the diff.

Comment thread src/utils/pythonRunner.ts
@@ -200,12 +212,36 @@ export async function runEntrypoint(
if (exitCode === 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Timeout kills only the shim; a null exit code is treated as success

Anchored here because L210 (const exitCode = code ?? 0) is unchanged in this PR, so it isn't in the diff.

runPythonProgram maps a null close code (child killed by a signal, including the 300 s spawn-timeout SIGTERM) to exit 0 via const exitCode = code ?? 0, and the PR now routes lineage generate through it. The timeout kills only the python -c shim; the SDK's generate_lineage does the real work in a child worker that inherits the CLI's stdio pipes, so the worker is orphaned, keeps running, and the 'close' event is delayed until it exits (or never). (In forcedotcom/datacloud-customcode-python-sdk#160, datacustomcode/lineage/__init__.py starts the worker with subprocess.run([sys.executable, "-m", "datacustomcode.lineage._worker", request_file], env=env, check=False): no timeout, inherited stdout/stderr.)

Failure scenario: A user runs sf data-code-extension script lineage generate on a script that takes more than 5 minutes or stalls on the network. At 300 s Node SIGTERMs only the shim; the worker keeps running and printing and the CLI keeps waiting, so the cap bounds nothing and a hung script hangs the CLI until Ctrl+C. When the worker finally ends, code ?? 0 turns the null code into success, lineage.json does not exist, and the user gets 'The lineage run finished but produced no result: ENOENT...' with advice to upgrade the SDK and no mention of a timeout (reproduced with the PR-head runner against the SDK's shim/worker layout: close fired only when the worker exited). The same line makes script run print 'completed successfully' for a script killed at the 5-minute cap or OOM-killed. The line predates the PR but is now shared by the new command.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb, two parts:

  • runPythonProgram no longer maps a null close code to 0. A signal-terminated run (timeout, OOM kill) now rejects with RunTerminated ("stopped by signal X before it finished. Runs time out after N seconds"), so script run gets the fix too.
  • The lineage shim installs a SIGTERM handler that raises SystemExit. Python's subprocess.run kills the child on any exception while waiting, so the SDK worker dies with the shim, the pipes close, and the timeout actually bounds the run. The shim exits 143 with a stderr marker, which maps to the same RunTerminated error; a script's own exit 143 without the marker is not mistaken for it.

I confirmed this with a real process: a stand-in shim with a 20 s worker under a 2 s timeout closed at 20.6 s with code null before the change, and at 2.0 s with code 143 (no orphan) after.

Known gap: on Windows, Node's kill() is TerminateProcess, so the handler never runs and the worker can still be orphaned. A process-tree kill (taskkill /T, or a process-group kill on POSIX with SIGINT forwarding) changes how run spawns too, so I'm tracking it as a separate follow-up.

Comment thread src/base/lineageBase.ts
const needsOwnership = !flags['accept-lineage-ownership'];
const needsReplace = replacing && !flags['no-prompt'];
if (!needsOwnership && !needsReplace) return true;
if (this.jsonEnabled() || !process.stdin.isTTY) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Write refusal under --json/non-TTY happens only after the full run

The refusal to write under --json or a non-TTY stdin (LineageOwnershipNotAccepted, then LineageReplaceNotConfirmed) is raised only inside confirmWrite, after the whole org-backed run, and it reports one missing flag per attempt. The generated lineage is discarded unless --output-file was given, and the thrown error carries no dataLineage.

Failure scenario: A CI step runs sf data-code-extension script lineage generate -e payload/entrypoint.py -o myorg --json without --accept-lineage-ownership. It authenticates and runs the customer script for up to 5 minutes, then exits 1 with LineageOwnershipNotAccepted. If config.json already has a different dataLineage, adding --accept-lineage-ownership costs another full run that ends in LineageReplaceNotConfirmed asking for --no-prompt, so a replacing package burns two failed full runs before the third succeeds. Empty, dry-run and unchanged outcomes legitimately succeed without the flags, so a blanket pre-run check would regress them; but for a first-time package (no dataLineage section) every non-empty result needs the flags, so the late failure is avoidable there, and the generated lineage could at least be emitted before the write is refused.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb. Under --json or a non-TTY stdin, and without --dry-run, the command now fails before the run when --accept-lineage-ownership is missing, and also when --no-prompt is missing and config.json already has a dataLineage section.

You're right that this drops the empty and unchanged exit-0 outcomes in that mode. That's deliberate: --dry-run covers "check whether lineage is current" (it never prompts, exits 0, and --json includes diff and entryCount), and the error's actions now point there. The late check in confirmWrite stays as a backstop in case config.json gains a dataLineage section during the run.

Comment thread src/base/lineageBase.ts Outdated
);
}
// Fail before the run rather than lose its result to a bad path afterwards.
await fs.access(path.dirname(outputFile), fsConstants.W_OK).catch((err: Error) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--output-file target isn't validated before the run

The only pre-run validation of --output-file is the package-input collision check plus fs.access(path.dirname(outputFile), W_OK). Nothing checks the target itself, so a directory, a read-only existing file, or a path under a regular file passes both checks and the unguarded fs.writeFile at line 149 fails only after the run. The parent-only W_OK test also wrongly rejects writable special targets such as /dev/stdout for a non-root user.

Failure scenario: A user runs lineage generate -e ... -o myorg --dry-run --output-file . (or --output-file ./reports where reports/ exists, or a read-only file left by a previous run). Both pre-checks pass and the org-backed run executes for up to 5 minutes, then the command dies with a raw Error (EISDIR) (or EACCES/ENOTDIR) with no actions. The summary, diff, dry-run output, prompts and config.json write never happen and the in-memory result is lost, so the user must fix the path and repeat the whole run (reproduced for a directory, a trailing-slash directory, '.', a 0444 file and a path under a file). Conversely --output-file /dev/stdout is rejected up front with LineageOutputFileUnwritable although the write would succeed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb. Before the run, an existing --output-file must be a regular file with W_OK (a directory, ., or a trailing-slash path now fails with LineageOutputFileUnwritable: ... not a regular file), and a new path still needs a writable parent. Tests cover a directory and a 0444 file.

I left the /dev/stdout case as is: it isn't a regular file, so it's still rejected up front, which is the safer side of the trade.

Comment thread src/base/lineageBase.ts Outdated
};

if (entries.length === 0) {
this.warn(messages.getMessage('warning.emptyResult'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Empty trace leaves a stale dataLineage in place and reports success

When the trace yields zero entries but config.json already has dataLineage entries, the command prints '-N removed', warns that the package 'follows the no-lineage path', and returns without writing or offering anything, so the existing entries still ship at deploy. The early return also produces the same success:true, written:false, 'Lineage generation completed successfully!' result and exit code 0 as a real write.

Failure scenario: A script's writes are all literals or untraceable pandas/mapInPandas columns, or it was edited so previously traced outputs are now unresolved, while config.json still carries dataLineage entries (hand-added or from an earlier generate). lineage generate prints '-3 removed', warns 'config.json was not modified ... follows the no-lineage path' and exits 0. The user believes no lineage applies, but deploy zips config.json as-is, so the stale entries still drive governance tag propagation or hit the 1041/1042 permission validation, and the CLI never offers to clear them. In CI (--accept-lineage-ownership --no-prompt --json) the pipeline sees exit 0, success:true and 'completed successfully' with only a warning in warnings[], unless it parses written/entryCount.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Partly addressed in 8269aeb. When the trace is empty but config.json already has entries, the command now shows a separate warning: config.json was not modified, its existing N dataLineage entries still apply and are deployed with the package, review or remove them. It no longer claims the no-lineage path.

We chose to warn rather than offer to clear the entries. An empty trace is weak evidence the existing (possibly hand-written) lineage is wrong, and a clear is easy to do by hand. Exit code stays 0 since nothing failed, and CI can still read written / entryCount.

Comment thread src/base/lineageBase.ts
diff: LineageDiff,
gaps: LineagePermissionGaps
): void {
this.log(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replace prompt shows only counts, not the affected entries

The generated lineage replaces the whole dataLineage section, and report() prints only +N added, -N removed, ~N changed counts; the confirm prompt never says which entries are dropped or changed. The replace prompt also fires for additions-only diffs, and a config that is merely malformed or case-duplicated shows +0/-0/~0 with a replace prompt and no explanation.

Failure scenario: A user follows warning.udf / warning.unresolved and hand-adds a lookup-table source for a UDF target plus an entry for an unresolved target. After a script edit they re-run generate and see +0 added, -1 removed, ~1 changed with only the generic 'hand edits will be lost' warning. Answering yes, or running CI with --accept-lineage-ownership --no-prompt, reverts the UDF target to its traced arguments and deletes the unresolved target's entry, so tag propagation from those sources stops until the entries are re-added; declining wastes the multi-minute run, and seeing which entries were affected takes another run with --json, --dry-run or --output-file. An additions-only diff (+3/-0/~0) against an existing section likewise fails in CI with LineageReplaceNotConfirmed until --no-prompt is added.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb. Below the counts, the summary now lists the target fields the generated lineage drops and changes, so you can see what the replace affects before answering (or in CI logs).

Prompting on an additions-only diff is intended: the command replaces the whole section, so any existing section gets the replace confirmation.

Comment thread src/base/lineageBase.ts
messages.getMessages('actions.lineageChanged')
);
}
await writeConfigJson(configPath, replaceLineage(latest, entries));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No writability pre-check for config.json

config.json, the primary write target, gets no writability pre-check, although the PR adds one for the optional --output-file with the comment 'Fail before the run rather than lose its result to a bad path afterwards'. The read at line 90 proves only readability; writeConfigJson (a bare fs.writeFile) runs after the full run and the accepted prompt.

Failure scenario: config.json is read-only (Windows read-only attribute, chmod -w, read-only bind mount). The user pays for the environment check, org auth, the package run (up to 5 minutes), reads the diff and answers the replace prompt (which can wait up to 10 minutes). Only then does the write fail with a raw EACCES: permission denied, open '.../config.json' and no actions. Non-dry-run output prints only counts, so the generated lineage is lost unless --output-file was also given, and the whole run must be repeated (reproduced with a mirror of writeConfigJson against a chmod 444 config.json: read succeeds, write fails with EACCES).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb. Unless --dry-run is set, config.json gets a W_OK check before the run and fails with LineageConfigUnwritable. Dry runs don't write, so they still work on a read-only config.json (tested).

Comment thread messages/lineage.md Outdated

# flags.dependencies.description

Optional comma-separated list of Python package dependencies to use during the run, overriding those defined in the package's requirements.txt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--dependencies help misdescribes what the flag does

The hidden command's --dependencies help is copied verbatim from run: it says the flag lists Python package dependencies 'overriding those defined in the package's requirements.txt'. The SDK's import_dependencies only runs importlib.import_module on each entry (or module.attribute); it installs nothing and overrides nothing, so pip specifiers and distribution names that differ from import names fail.

Failure scenario: A user follows the help and runs lineage generate -e <entrypoint> -o <org> --dependencies 'pandas==2.0,requests>=2' (or passes scikit-learn, pyyaml, beautifulsoup4). The worker raises ModuleNotFoundError ("No module named 'pandas==2'") before the entrypoint runs; mapLineageExitCode does not handle it, so the CLI reports The package run failed: <traceback> (RunExecutionFailed) with generic advice about scan and installed dependencies, never pointing at the flag. Bare names equal to the import name work only if already installed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb, both here and in run.md where it was copied from. The flag is now described as modules (or module.attribute) to import before the entrypoint runs, using import names (e.g. yaml, not pyyaml), and nothing is installed.

Comment thread src/utils/lineageConfig.ts Outdated

/** Write config.json with a 2-space indent and a trailing newline. */
export async function writeConfigJson(configPath: string, config: Record<string, unknown>): Promise<void> {
await fs.writeFile(configPath, `${JSON.stringify(config, null, 2)}\n`, 'utf8');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

config.json trailing newline differs from scan/init

The new writeConfigJson appends a trailing newline, while script scan (nativeScan.ts:545), script init (nativeInit.ts writeJson) and the Python SDK write config.json without one. Each tool flips the other's byte format; scan.test.ts:289 pins the no-newline form while the lineage tests pin the newline form.

Failure scenario: A user runs script scan, then script lineage generate (confirms the write), commits, edits code, and runs script scan again without changing permissions. The re-scan rewrites config.json without the trailing newline, so git shows a one-line } change with a 'No newline at end of file' marker, and the next lineage write flips it back. Editors or pre-commit hooks that enforce a final newline fight scan. Nothing breaks at deploy or run time; the cost is diff noise and a third serializer to keep in sync (nativeScan.ts:545, nativeInit.ts:269, lineageConfig.ts:157).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb. writeConfigJson no longer appends a trailing newline, matching scan, init and the SDK, and the tests are updated to match.

if (missing.length > 0) {
throw invalidResult(`missing or non-array ${missing.join(', ')}`);
}
return report as LineageReport;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SDK lineage entries are cast without shape validation

parseLineageReport validates only that six top-level keys are arrays, then casts the SDK JSON to LineageReport unchecked. Entry shapes (targetField/sourceFields, reason, string targets/sources) are never validated, although readLineage already has a per-entry validator that is not applied to the SDK result.

Failure scenario: An SDK build whose to_dict() keeps the six array keys but changes element shapes (dataLineage entries without sourceFields, non-string targets/sources) passes parsing. After the multi-minute run, --output-file receives the unusable payload and the command exits 1 with a raw TypeError: Cannot read properties of undefined (reading 'map') or name.toLowerCase is not a function from diffLineage, report, countPairs or findPermissionGaps, instead of LineageResultInvalid with the pip-upgrade action in messages/lineage.md. The merged SDK matches the type today, so this is hardening against drift rather than a current failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed this is hardening against SDK drift rather than a current failure. The merged SDK's to_dict() matches the type, and the result-file contract is pinned by the shim. I'm leaving it as a follow-up rather than growing this PR; the natural fix is to run each entry through the same per-entry validator readLineage uses and raise LineageResultInvalid on mismatch.

Comment thread src/utils/lineageRunner.ts Outdated
}
return parseLineageReport(raw);
} finally {
await fs.rm(tempDir, { recursive: true, force: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Temp-dir cleanup failure can mask the run result

The finally in generateLineage does a bare await fs.rm(tempDir, { recursive: true, force: true }) with default maxRetries 0. If removal fails (transient EBUSY/EPERM from antivirus or an indexer on the fresh lineage.json, mainly Windows), the rejection replaces the already-parsed report or the in-flight SfError. zipBuilder.ts wraps its temp-dir cleanup in try/catch with a debug log for exactly this reason.

Failure scenario: A Windows user's antivirus briefly locks the freshly written temp lineage.json. The org-backed run (up to 5 minutes) completes and parses fine, then rm throws EBUSY/EPERM in finally. The command fails with a raw 'EBUSY: resource busy or locked, unlink ...', nothing is written to --output-file or config.json, and the user must repeat the whole run. A try/catch with debug logging, or maxRetries/retryDelay, avoids it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8269aeb. The cleanup is wrapped in try/catch with a debug log, the same pattern zipBuilder.ts uses, so a transient EBUSY/EPERM can no longer replace the parsed report or the in-flight error.

@joroscoSF
joroscoSF merged commit a2c9ab7 into main Oct 16, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants