Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First version of initial-analysis and inspect-* tools #9

Merged
merged 76 commits into from
Jan 13, 2025

Conversation

Domiii
Copy link
Contributor

@Domiii Domiii commented Dec 12, 2024

@Domiii Domiii self-assigned this Dec 12, 2024
@Domiii Domiii changed the title WIP: initial-analysis First version of initial-analysis and inspect-* tools Dec 17, 2024
…nik/pro-939-investigate-data-flow-potential-for-10608
@Domiii Domiii requested review from toshok and bhackett1024 January 10, 2025 14:42
// We need a recordingId either from the legacy prompt or from the new mode.
const { recordingId } = scanReplayUrl(options.prompt);

if (shouldUseLegacyMode(recordingId)) {
Copy link
Contributor Author

@Domiii Domiii Jan 10, 2025

Choose a reason for hiding this comment

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

NOTE: Only use the new workflow on selected recordings for now.
We generally want to remove this check once we can confirm that it works in all previous samples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This is a json-in-json-out CLI wrapper to avoid bash encoding for tool calls via CLI.

);

// 1. Hackfix-override `argv`.
process.argv = [process.argv[0], process.argv[1], input.command, ...flattenedArgStrings];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Convert json to argv (because commander has no json mode).

@@ -103,8 +103,9 @@ export async function annotateExecutionPoints(
const filePath = getFilePath(spec.repository, location.url);
const lines = fs.readFileSync(filePath, "utf8").split("\n");
let found = false;
const targetLine = location.source.trim();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: For some reason, the source line is not trimmed anymore. I only fixed this because it caused issues when re-testing 10609. 🤷‍♀️


export const ExecutionPointSpecSchema = AnalysisDefaultSpecSchema;
export const ExecutionPointSpecSchema = AnalysisDefaultSpecSchema.extend({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Not sure why these props were removed

*/
function ignoreMultiMediaImports() {
const fileUrl = pathToFileURL(__filename).toString();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: We need this, so we can deal with multimedia imports (import myImage from "myImage.svg";) from our devtools dependency.

@@ -41,6 +41,9 @@ export default class LocalGitRepo {

async init(forceDelete: boolean): Promise<void> {
if (forceDelete) {
if (!this.url) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: forceDelete is a legacy option, only needed when we annotate (and want to *re-*annotate) code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This test passes 🎉

@@ -0,0 +1,278 @@
import { ArrayPreview, DefaultPreview, ObjectPreview, ValuePreview } from "./values";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This was the old script I used during my experiments. It has some advantages over our default preview rendering, but not worth the effort until we have better benchmarks.

Copy link
Contributor

@bhackett1024 bhackett1024 left a comment

Choose a reason for hiding this comment

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

@Domiii we need to have a discussion about system architecture here. There is a ton of functionality here that is similar to / duplicative of stuff we have in the backend's analysis logic, and neither of these can use each other because they run in totally different environments.

debug(JSON.stringify({ experimentalCommand: { command, spec: input.spec } }) + "\n");

// const timeout = 15 * 1000;
const timeout = 30 * 1000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a timeout.

return (process.env[envVar]?.length && process.env[envVar] !== "0") || false;
}

export function isReplayDevMode(): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added REPLAY_DEV_MODE to make a few things a bit easier.

@Domiii Domiii merged commit eb7fe42 into main Jan 13, 2025
@Domiii Domiii deleted the dominik/pro-939-investigate-data-flow-potential-for-10608 branch January 13, 2025 11:48
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