Skip to content

Implement ship target list#557

Open
jsdavid278-cyber wants to merge 1 commit into
profullstack:masterfrom
jsdavid278-cyber:patch-2
Open

Implement ship target list#557
jsdavid278-cyber wants to merge 1 commit into
profullstack:masterfrom
jsdavid278-cyber:patch-2

Conversation

@jsdavid278-cyber
Copy link
Copy Markdown

No description provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

This PR replaces the sh1pt ship target list stub with a working implementation that reads sh1pt.config.ts (or a custom --config path) and parses its targets block using a hand-rolled regex/brace-matching pipeline, then renders results as a coloured table or --json output.

  • The new readTargetSummary / readObjectBody pipeline does not strip comments before running its regex, so a config file that contains targets: in a comment (e.g. an inline example or JSDoc) will have that comment's body parsed as the real targets block instead of the actual property.
  • readStringProperty uses [^'\"]+ which forbids both quote characters from the captured value, silently truncating string values that contain the opposite quote type.
  • The consecutive-backslash escape check (prev !== '\\\\') in stripComments and findMatchingBrace will misidentify the end of a string when a backslash-escaped backslash (\\\\) immediately precedes a closing quote.

Confidence Score: 3/5

Safe to merge only after fixing the comment-stripping gap in readObjectBody; the feature will silently misreport targets for any config file that mentions targets: in a comment.

The entire feature hinges on readObjectBody correctly locating the targets block. Because it runs the regex against the raw source without first stripping comments, any inline example or description comment that contains targets: { … } causes the function to parse the comment's mock object and ignore the real one — the target list command then shows wrong (or empty) data with no error.

All changes are in packages/cli/src/commands/ship.ts, specifically the readObjectBody function and its interaction with the comment-stripping utilities added in the same file.

Important Files Changed

Filename Overview
packages/cli/src/commands/ship.ts Replaces the target list stub with a real implementation that reads and parses sh1pt.config.ts; adds ~130 lines of hand-rolled TS/JS source parsing. The parsing pipeline has a correctness gap: readObjectBody does not strip comments before matching, so a comment containing targets: { … } causes the wrong block to be returned. Two additional parsing edge cases exist around mixed-quote string values and the consecutive-backslash escape check.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as ship target list
    participant rTS as readTargetSummary
    participant rOB as readObjectBody
    participant rTLE as readTopLevelObjectEntries
    participant SC as stripComments
    participant FS as node:fs

    User->>CLI: sh1pt ship target list [--json] [--config path]
    CLI->>rTS: cwd, configPath
    rTS->>FS: existsSync(path)
    alt file missing
        rTS-->>CLI: throw Error
        CLI-->>User: stderr + process.exit(1)
    else file exists
        FS-->>rTS: ok
        rTS->>FS: readFileSync(path, 'utf8')
        FS-->>rTS: source (raw)
        rTS->>rOB: source, 'targets'
        Note over rOB: ⚠️ regex runs on raw source (comments not stripped here)
        rOB-->>rTS: "body string | undefined"
        alt no targets block
            rTS-->>CLI: []
            CLI-->>User: No targets configured.
        else targets body found
            rTS->>rTLE: body
            rTLE->>SC: body
            SC-->>rTLE: stripped body
            rTLE-->>rTS: "Array of {key, body}"
            rTS-->>CLI: TargetSummary[]
            alt --json flag
                CLI-->>User: JSON.stringify output
            else human output
                CLI-->>User: coloured target table
            end
        end
    end
Loading

Reviews (1): Last reviewed commit: "Implement ship target list" | Re-trigger Greptile

Comment on lines +177 to +184
function readObjectBody(source: string, property: string): string | undefined {
const match = new RegExp(`(?:^|[,{\\s])${escapeRegExp(property)}\\s*:`).exec(source);
if (!match) return undefined;
const open = source.indexOf('{', match.index + match[0].length);
if (open === -1) return undefined;
const close = findMatchingBrace(source, open);
return close === -1 ? undefined : source.slice(open + 1, close);
}
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.

P1 readObjectBody can match targets: inside a comment

readObjectBody runs its regex against the raw, unstripped source, so any comment containing the word targets: — such as // Example: targets: { ios: { use: '…' } } or a JSDoc block — will be matched first. The function then calls source.indexOf('{', …) on the raw source, which finds the { belonging to the comment's inline object. findMatchingBrace then faithfully walks that comment's braces and returns the comment's body as the targets block. The downstream readTopLevelObjectEntries call strips comments from THAT body, but it's already operating on the wrong slice of text, so the real targets are never surfaced. The fix is to run stripComments on the source before the regex match — the same thing readTopLevelObjectEntries does for its input.

Comment on lines +291 to +294
function readStringProperty(source: string, key: string): string | undefined {
const match = new RegExp(`${escapeRegExp(key)}\\s*:\\s*['"]([^'"]+)['"]`).exec(source);
return match?.[1];
}
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.

P2 readStringProperty uses [^'"]+ in the character class, which forbids BOTH single and double quotes from appearing inside the captured value. This means a value like use: "foo'adapter" would silently capture only foo (the regex closes on the embedded ' before the closing "), returning wrong data with no error. Matching only the delimiter that opened the string prevents this class of truncation.

Suggested change
function readStringProperty(source: string, key: string): string | undefined {
const match = new RegExp(`${escapeRegExp(key)}\\s*:\\s*['"]([^'"]+)['"]`).exec(source);
return match?.[1];
}
function readStringProperty(source: string, key: string): string | undefined {
const match = new RegExp(`${escapeRegExp(key)}\\s*:\\s*(?:'([^']*)'|"([^"]*)")`).exec(source);
return match?.[1] ?? match?.[2];
}

Comment on lines +228 to +229
if (ch === quote && prev !== '\\') quote = undefined;
continue;
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.

P2 Consecutive-backslash escape check is incorrect in stripComments

The check prev !== '\\' uses only the immediately preceding character to decide whether a closing quote is escaped. This fails for double-backslash sequences: in the literal 'hello\\', when the parser reaches the closing ', prev is \, so it treats the quote as escaped and never closes the string. The string tracking then "leaks" into subsequent file content, potentially hiding real properties from the parser. A correct check counts the run of consecutive backslashes before the quote; if the run is even, the quote is not escaped.

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.

1 participant