Implement ship target list#557
Conversation
Greptile SummaryThis PR replaces the
Confidence Score: 3/5Safe to merge only after fixing the comment-stripping gap in The entire feature hinges on All changes are in Important Files Changed
Sequence DiagramsequenceDiagram
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
Reviews (1): Last reviewed commit: "Implement ship target list" | Re-trigger Greptile |
| 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); | ||
| } |
There was a problem hiding this comment.
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.
| function readStringProperty(source: string, key: string): string | undefined { | ||
| const match = new RegExp(`${escapeRegExp(key)}\\s*:\\s*['"]([^'"]+)['"]`).exec(source); | ||
| return match?.[1]; | ||
| } |
There was a problem hiding this comment.
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.
| 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]; | |
| } |
| if (ch === quote && prev !== '\\') quote = undefined; | ||
| continue; |
There was a problem hiding this comment.
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.
No description provided.